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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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 ?

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

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

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

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

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 ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel