|
From: | Gerd Hoffmann |
Subject: | Re: [Qemu-devel] [PATCH 00/10] AHCI emulation support v2 |
Date: | Fri, 19 Nov 2010 12:56:53 +0100 |
User-agent: | Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.9) Gecko/20100827 Red Hat/3.1.3-1.el6 Thunderbird/3.1.3 |
Hi,
As I would rather have something working we can base on in the tree, so whoever volunteers for the refactoring (hint!) knows how to design the interfaces, I am not sure how much is reasonable within this patch set.I guess I have to read this as: You want to drop the code into the repository, no matter what mess it creates, and leave the cleanup to some volunteer that will never appear. So whoever is in the unfortunate position of having to touch the IDE code afterwards will have the pain because this series is doing only the half of what should be done.
Well, this is how IDE gets cleaned up right now. I was one of the victims ;)
IDE is in a noticeable better state than it used to be two years ago. It is still quite messy though.
I'm sure that with a working time of only a few days the result could be substantially improved, even if a month would be needed for perfection.
[ disclaimer: didn't look at v3 yet ] Adding ahci should at least not make it more messy that it already is. Doing the controller-depending dispatching through a function pointer table (aka IDEBusOps) looks sane to me. SCSI does the same, although without a Ops struct as it needs a single function pointer only. Moving code shared between sata/ahci and ide to another source file (ata.c ?) makes sense and would be a good start to make clear which code is used for what. IDEBus must learn some things about sata. For example there is no such thing as a slave device, so a IDEBus representing a sata port must not allow slave devices being attached. Fixing the IDEState mess would be great. Right now IDEBus carries a array of IDEStates for master and slave for historical reasons: The ide code (used to?) assume that the IDEStates for master and slave are allocated together and that it can easily jump from one to the other and back using pointer arithmetics on the IDEState struct. IDEState doesn't belong there though, it should be part of IDEDrive. Also having a unused slave state doesn't make sense at all for sata. Fixing that without breaking migration could be non-trivial though. cheers, Gerd
[Prev in Thread] | Current Thread | [Next in Thread] |