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 ("Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts"):
>> Per suggestion, along with the latest patch is a more detailed
>> description to add to the check-in message -
> 
> It looks good to me, thanks, apart from some nits.  If you could
> address these I'll apply it :-).
> 
>> -            if (libxl__blktap_enabled(&gc))
>> +            if (libxl__blktap_enabled(&gc) && 
>> +                 disk->format != DISK_BACKEND_QDISK)
> 
> Some mistake here surely ?
>
Of course!

>> diff -r 6c22ae0f6540 tools/libxl/libxl.c
>> --- a/tools/libxl/libxl.c	Fri Feb 11 16:51:44 2011 +0000
>> +++ b/tools/libxl/libxl.c	Fri Feb 11 13:48:12 2011 -0500
> ...
>> -    switch (disk->phystype) {
> ...
>> +    switch (disk->backend) {
> 
> You have introduced these braces { }, but that seems to me to be just
> a stylistic change and they are not really needed ?
> 
No, I have switched disk->phystype to disk->backend.

>> +            libxl__device_physdisk_major_minor(disk->pdev_path, &major, &minor);
> 
> This function can fail.  It has two call sites neither of which check
> the return value.  Perhaps that should be addressed in a separate
> patch, as your change here isn't making it worse.  So no need to do
> anything about this right now if you don't want to.
> 
Yes, it isn't part of what we intended to change.  There are other things too
with respect to disk configuration option code that requires revisiting.

>> -        case PHYSTYPE_QCOW2:
>> -        case PHYSTYPE_VHD:
>> +        case DISK_BACKEND_TAP:
>> +        case DISK_BACKEND_QDISK: {
> 
> Once again, this is a stylistic change.  These { } are not used
> elsewhere in libxl so you should not introduce them.  (It would be a
> different matter if there were some reason why they are required in a
> particular case, eg to introduce a relevant block scope.)
> 
I agree, will change.

>>          case DSTATE_PHYSPATH:
>> -            if ( *p == ',' ) {
>> +            if (*p == ',') {
>>                  int ioemu_len;
> 
> It is good that you fixed up the formatting in code you changed, but
> please do not make formatting changes to unchanged lines.
>
Argg...  I was worried you might ask why I didn't fix the style around the code
I changed.  I have reverted the changes.

> We will do a style cleanup after 4.1 is released.
> 
>> -                if ( tok + ioemu_len < end &&
>> +                if (tok + ioemu_len < end &&
> 
> Once again.  Can you make sure your patch doesn't have /any/ unrelated
> formatting changes to lines you don't touch, please ?
>
Yes.

I will send a patch momentarily.

Kamala