Message-ID: <4EBBD503.4020308@codemonkey.ws>
Date:	Thu, 10 Nov 2011 07:43:31 -0600
From:	Anthony Liguori <anthony@...emonkey.ws>
To:	Pekka Enberg <penberg@...nel.org>
CC:	Pekka Enberg <penberg@...helsinki.fi>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Avi Kivity <avi@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	kvm@...r.kernel.org, Christoph Hellwig <hch@....de>
Subject: Re: [RFC/GIT PULL] Linux KVM tool for v3.2

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@linux.vnet.ibm.com

Regards,

Anthony Liguori
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists