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

[Soren Hansen]

On Tue, Apr 29, 2008 at 02:27:00PM +0100, Daniel P. Berrange wrote:
> > +        strcmp((const char *)target, "hdd") &&
> > +        strcmp((const char *)target, "hdd") &&
> 
> These two lines test the same thing !

Quite so. My bad.

> > +        strncmp((const char *)target, "vd", 2)) {
> 
> Its probably a better idea to just compress the explicit hda -> hdd 
> comparisons down into a single 'hd' prefix comparison, as you did
> with 'vd'.

Hm.. Yes, I suppose that if you're using -drive notation, "hd[e-z]"
starts making sense. Fixed.

>> +    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?

IMO, it's better to change this sort of things all in one go, and keep
everything consistent until then.

>> +    else {
>> +        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "Invalid bus type: %s", bus);
>> +        goto error;
>> +    }
> 
> This line needs to be marked for translation, with _(...). 

Fixed.

> 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.

> > +static char *qemudAddBootDrive(virConnectPtr conn,
> > +                                struct qemud_vm_def *def,
> > +                            	char *handledDisks,
> > +                            	int type) {
> > +    int j = 0;
> > +    struct qemud_vm_disk_def *disk = def->disks;
> > +
> > +    while (disk) {
> > +        if (!handledDisks[j] && disk->device == type) {
> > +            handledDisks[j] = 1;
> > +            return qemudDriveOpt(disk, 1);
> > +        }
> > +        j++;
> > +        disk = disk->next;
> > +    }
> > +    qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> > +                         "Requested boot device type %d, but no such device defined.", type);
> > +    return 0;
> > +}
> >  
> 
> > @@ -2207,30 +2295,32 @@
> >              goto no_memory;
> >      }
> >  
> > -    for (i = 0 ; i < vm->def->os.nBootDevs ; i++) {
> > -        switch (vm->def->os.bootDevs[i]) {
> > -        case QEMUD_BOOT_CDROM:
> > -            boot[i] = 'd';
> > -            break;
> > -        case QEMUD_BOOT_FLOPPY:
> > -            boot[i] = 'a';
> > -            break;
> > -        case QEMUD_BOOT_DISK:
> > -            boot[i] = 'c';
> > -            break;
> > -        case QEMUD_BOOT_NET:
> > -            boot[i] = 'n';
> > -            break;
> > -        default:
> > -            boot[i] = 'c';
> > -            break;
> > +    if (vm->def->virtType != QEMUD_VIRT_KVM) {
> 
> This is wrong - both QEMU and KVM can support the -drive parameter,

Oh, I wasn't aware. Hmm..

> and not all KVM support it. You need to add a check in the method
> qemudExtractVersionInfo() to probe for availability of the -drive
> option, as we do with other opts - see QEMUD_CMD_FLAG_NO_REBOOT for
> example.

Interesting. I'll add that.

> 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?

> 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. 

>> +int virDiskNameToIndex(const char *name) {
>> +    const char *ptr = NULL;
>> +    int idx = 0;
>> +
>> +    if (strlen(name) < 3)
>> +        return 0;
>> +
>> +    switch (*name) {
>> +        case 'f':
>> +        case 'h':
>> +        case 'v':
> You need a case 's' there to allow for SCSI...

Good point. I suppose I do so in qemudParseDiskXml as well (when
checking the target).

-- 
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/9e98ce35/attachment-0001.sig>