On 11/10/2011 12:46 AM, Pekka Enberg wrote:
Hi Anthony,
1) The RTC emulation is limited to emulating CMOS and only the few fields used
to store the date and time. If code is added to arch/x86 that tries to make
use of a CMOS field for something useful, kvm-tool is going to fall over.
None of the register A/B/C logic is implemented and none of the timer logic is
implemented. I imagine this requires kernel command line hackery to keep the
kernel from throwing up.
The "fake it until you make it" design principle is actually something Ingo
suggested early on and has been a really important factor in getting us to where
we are right now.
Not that I disagree with you. I think we should definitely clean up our hardware
emulation code.
If a kernel change that works on bare metal but breaks kvm-tool because
kvm-tool is incomplete is committed, is that a regression that requires
reverting the change in arch/x86?
If it's the KVM tool being silly, obviously not.
2) The qcow2 code is a filesystem implemented in userspace. Image formats are
file systems. It really should be reviewed by the filesystem maintainers.
There is absolutely no attempt made to synchronize the metadata during write
operations which means that you do not have crash consistency of the meta data.
If you experience a power failure or kvm-tool crashs, your image will get
corrupted. I highly doubt a file system would ever be merged into Linux that
was this naive about data integrity.
The QCOW2 is lagging behind because we lost the main developer. It's forced as
read-only for the issues you mention. If you think it's a merge blocker, we can
drop it completely from the tree until the issues are sorted out.
It's not just the qcow2 implementation or even the block layer. This pull
requests adds a userspace TCP/IP stack to the kernel and yet netdev isn't on the
CC and there are no Ack's from anyone from the networking stack. I'm fairly
sure if they knew what was happening here they would object.
And the implementation isn't even strictly needed. You can just as well achieve
the same goal using tun/tap with a privileged helper[1].
I found these three issues in the course of about 30 seconds of looking
through the kvm-tool code. I'm sure if other people with expertise in these
areas looked through the code, they would find a lot more issues. I'm sure I
could find many, many more issues.
Thanks for the review!
Would you be interested in spending another 30 seconds to find out some more
issue? :-)
I could, provided you could take the things you want to do differently and
submit them as patches to qemu.git instead of creating a new tool.
There are lots of people on qemu-devel than can provide deep review of this type
of code. That's the advantage of working in qemu.git.
[1] http://mid.gmane.org/1320086191-23641-1-git-send-email-coreyb@xxxxxxxxxxxxxxxxxx
Regards,
Anthony Liguori
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
