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

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Wed, 9 Feb 2011, Kamala Narasimhan wrote:
> Description:
> 
> This patch refactors xl disk specific interface to separate disk format and backend types, rename some variables, changes code that is impacted by this interface change.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 
> Kamala
> 
> PS:  This patch incorporates all the comments made so far that are specific to this patch.

Better, we are almost there.
The only problems I have found are in the changes to
libxl_device_disk_local_attach:


> @@ -1052,36 +1051,44 @@ char * libxl_device_disk_local_attach(li
>      libxl__gc gc = LIBXL_INIT_GC(ctx);
>      const char *dev = NULL;
>      char *ret = NULL;
> -    int phystype = disk->phystype;
> -    switch (phystype) {
> -        case PHYSTYPE_PHY: {
> -            fprintf(stderr, "attaching PHY disk %s to domain 0\n", disk->physpath);
> -            dev = disk->physpath;
> +
> +    switch (disk->backend) {
> +        case DISK_BACKEND_PHY: {
> +            fprintf(stderr, "attaching PHY disk %s to domain 0\n", disk->pdev_path);
> +            dev = disk->pdev_path;
>              break;
>          }
> -        case PHYSTYPE_FILE:
> -            /* let's pretend is tap:aio for the moment */
> -            phystype = PHYSTYPE_AIO;
> -        case PHYSTYPE_AIO:
> -            if (!libxl__blktap_enabled(&gc)) {
> -                dev = disk->physpath;
> +        case DISK_BACKEND_TAP: {
> +            if (disk->format == DISK_FORMAT_VHD)
> +            {
> +                if (libxl__blktap_enabled(&gc))
> +                    dev = libxl__blktap_devpath(&gc, disk->pdev_path, disk->format);
> +                else
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapdisk2 is required to open a vhd disk\n");
> +                break;
> +            } else if (disk->format == DISK_FORMAT_QCOW ||
> +                       disk->format == DISK_FORMAT_QCOW2) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a qcow or qcow2 disk image\n");
> +                break;
> +            } else {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
> +                    "type: %d\n", disk->backend);
>                  break;
>              }
> -        case PHYSTYPE_VHD:
> -            if (libxl__blktap_enabled(&gc))
> -                dev = libxl__blktap_devpath(&gc, disk->physpath, phystype);
> -            else
> -                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapdisk2 is required to open a vhd disk\n");
> +        }
> +        case DISK_BACKEND_QDISK: {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a qdisk "
> +                "image\n");
>              break;
> -        case PHYSTYPE_QCOW:
> -        case PHYSTYPE_QCOW2:
> -            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a qcow or qcow2 disk image\n");
> +        }
> +        case DISK_BACKEND_UNKNOWN:
> +        default: {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
> +                "type: %d\n", disk->backend);
>              break;
> +        }
> +    }
>  
> -        default:
> -            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk physical type: %d\n", phystype);
> -            break;
> -    }
>      if (dev != NULL)
>          ret = strdup(dev);
>      libxl__free_all(&gc);
> 

The simple raw file case is not supported anywhere, besides it is
possible to attach a qdisk if it is in raw format.
It should be more or less like this:


phy && raw -> OK
phy && anything but raw -> KO
qdisk && raw -> OK
qdisk && anything but raw -> KO
tap && (qcow || qcow2) -> KO
tap && blktap2 enabled && (vhd || raw) -> OK
tap && blktap2 disabled && vhd -> KO
tap && blktap2 disabled && raw -> OK (same as qdisk)


> diff -r 1967c7c290eb tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Wed Feb 09 12:03:09 2011 +0000
> +++ b/tools/libxl/xl_cmdimpl.c	Wed Feb 09 10:57:42 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(physpath %s)\n", d_config->disks[i].pdev_path);
> +        printf("\t\t\t(phystype %d)\n", d_config->disks[i].backend);
> +        printf("\t\t\t(virtpath %s)\n", d_config->disks[i].vdev);
> 

we should print pdev_path, backend and vdev here