Re: [PATCH 2/5] Xl interface change plus changes to code it impacts

[Kamala Narasimhan <kamala.narasimhan@gmail.com>]

Ian Jackson wrote:
> Kamala Narasimhan writes ("[xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts"):
>> Attached are the changes made to xl disk related interface per earlier discussion.  Please let me know if there are further comments/issues to fix.
> 
> Thanks.  I have some comments of my own:
> 
>> +char *libxl__device_disk_string_of_backend(libxl_disk_backend backend)
> ...
>> +    switch (backend) {
>> +        case DISK_BACKEND_QEMU: return "qdisk";
>> +        case DISK_BACKEND_TAPDISK2: return "tap";
>> +        case DISK_BACKEND_BLKBACK: return "phy";
> 
> Perhaps the backend type number constants should be _QDISK, _TAP,
> _PHY ?  I think a function like _string_of should be a simple mapping
> to return the string version of the same name, not also change the
> name.
>
I can make that change.

>> -            if (libxl__blktap_enabled(&gc))
>> +            if ( libxl__blktap_enabled(&gc) && 
>> +                 disk->format != DISK_BACKEND_QEMU )
> 
> Don't add whitespace inside the if's ( ).  (You have done this in
> several places.  I know that libxl isn't entirely consistent but
> we have a defined coding style shouldn't be making the code less
> consistent.)
> 
Oh, boy!  I have consistently done that all over the patches as I assumed that
to be our convention.  Will change that too.

>> diff -r e4406b9fb064 tools/libxl/xl_cmdimpl.c
>> --- a/tools/libxl/xl_cmdimpl.c	Mon Feb 07 15:04:32 2011 +0000
>> +++ b/tools/libxl/xl_cmdimpl.c	Mon Feb 07 11:28:10 2011 -0500
>> @@ -361,9 +361,9 @@ static void printf_info(int domid,
>>          printf("\t\t(tap\n");
>>          printf("\t\t\t(backend_domid %d)\n", d_config->disks[i].backend_domid);
>>          printf("\t\t\t(domid %d)\n", d_config->disks[i].domid);
>> -        printf("\t\t\t(physpath %s)\n", d_config->disks[i].physpath);
>> -        printf("\t\t\t(phystype %d)\n", d_config->disks[i].phystype);
>> -        printf("\t\t\t(virtpath %s)\n", d_config->disks[i].virtpath);
>> +        printf("\t\t\t(pdev_path %s)\n", d_config->disks[i].pdev_path);
>> +        printf("\t\t\t(backend %d)\n", d_config->disks[i].backend);
>> +        printf("\t\t\t(vdev %s)\n", d_config->disks[i].vdev);
> 
> This part of the code is providing information which is intended to be
> parsed by callers which were written to cope with the output from xm.
> For backward compatibility, the previously used names and values
> should be output with the previously used semantics; it is OK to add
> new ones too with more sane semantics.
> 
> I think it's also acceptable to be a bit approximate with the
> emulation, but simply removing the old names is not correct.
> 
I didn't realize that.  Will keep the old display names then.

Kamala