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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

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

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

Ian.

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