[Libvir] [PATCH] Add bus attribute to disk target definition

[Soren Hansen]

On Tue, Apr 29, 2008 at 03:17:14PM +0100, Daniel P. Berrange wrote:
>>>> +    if (!bus)
>>>> +        disk->bus = QEMUD_DISK_BUS_IDE;
>>>> +    else if (!strcmp((const char *)bus, "ide"))
>>>> +        disk->bus = QEMUD_DISK_BUS_IDE;
>>>> +    else if (!strcmp((const char *)bus, "scsi"))
>>>> +        disk->bus = QEMUD_DISK_BUS_SCSI;
>>>> +    else if (!strcmp((const char *)bus, "virtio"))
>>>> +        disk->bus = QEMUD_DISK_BUS_VIRTIO;
>>> Can you use the   STREQ   macro here instead of strcmp.
>> Erm... I *could*.. I'm curious, though, why e.g. the similar code right
>> above it doesn't use STREQ if that's the preferred way to do it?
> We've been slowly updating code to match these new standards when doing
> patches.

Well, if that's the way you do it, I'll follow suit..  However, I have
to say that I pity the person that reads the code and finds these two
sections of code that seem to do rather similar things, but use
different functions to do it, and then has to work out what on earth the
difference between the two might be.

>>> You can run 'make syntax-check' for check for such issues.
>> Yes, in theory :)  In the real world, however, "make syntax-check"
>> fails horribly here. I'll be fixing that up next.
> If you send details to the list,  Jim will no doubt be able to point
> you in the right direction on this...

I'll do that in a minute. Thanks.

>>> Even if the -drive parameter is supported, it should still pass the
>>> -boot a/c/d/n  parameter in.
>> Why? And how would you boot from a virtio device this way?
> It is needed for PXE boot at least, and IMHO,

Good point.

> QEMU should treat 'boot c' as if  'boot=on' were set for the first
> -drive parameter for back compat.

Yes, indeed it should. Sadly, though, it doesn't.

  kvm -drive file=root.qcow,if=virtio -boot c

fails miserably, while 

  kvm -drive file=root.qcow,if=virtio,boot=on

works beautifully.

This logic is going to look horrible :( Something like:

if boot == hd && (one of the disks is a virtio disk)
then
	use new style -drive foo,boot=on notation
else
	use old style -boot [acdn] notation
	
?

>>> There is nothing in the -drive parameter handling, AFAICT, that
>>> requires the boot drive to be listed first on the command line. So
>>> this first loop is not needed, and this second loop is sufficient,
>>> simply turn on the boot= flag on the first match drive type when
>>> iterating over the list.
>> If you want to specify more than one boot device, the only way to
>> determine the priority is by specifying them ordered by priority. At
>> least, that's how it *ought* to be, but now I see that extboot only
>> actually supports one boot device. :/ I could have sworn I had seen
>> it work with both hard drive and cdrom. 
> The QEMU code only allows a single boot device & will abort if > 1 has
> it
> 
>             if (extboot_drive != -1) {
>                 fprintf(stderr, "qemu: two bootable drives specified\n");
>                 return -1;
>             }

Yes, that's what I noticed earlier today, although I geniunely hope this
is something that will be fixed at some point, and when that happy day
comes, I've either guessed correctly as to how it will derive boot
priorities and we won't have to fix anything, or I've guessed wrong, and
then we'll be no worse off than if we didn't adopt this approach right
now, AFAICS.

-- 
Soren Hansen               | 
Virtualisation specialist  | Ubuntu Server Team
Canonical Ltd.             | http://www.ubuntu.com/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20080429/570c14b4/attachment-0001.sig>