Re: [PATCH v2 6/9] libxl: Share logic for finding path between qemuu and pygrub

[George Dunlap <George.Dunlap@eu.citrix.com>]

On Mon, Mar 21, 2016 at 6:16 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> From: George Dunlap <george.dunlap@eu.citrix.com>
>
> qemu can also access disks which will be provided with a qdisk backend
> directly; add a flag to libxl__device_disk_find_local_path to indicate
> whether to check for qdisk direct access.
>
> Call libxl__device_disk_find_local_path() for most paths.  If we can't
> find a local path, print an error and skip the disk, rather than using
> a bogus path.
>
> Now if there is no local access to the disk (i.e., because the disk
> has a non-local backend, or relies on a custom hotplug script), libxl
> will now print a warning and not provide the emulated disk, rather
> than providing bogus parameters to qemu which cause it to error out.
> (Such disks will still be available via the PV backend.)
>
> I left the libxl__blktap_devpath in the qemuu-specific code rather
> than sharing it with the pyrgub code because:
>
> 1) When the pygrub path runs the guest disks have not yet been set up
>
> 2) libxl__blktap_devpath() will give you the existing devpath if it
> already exists, but will set one up for you if you don't.  So on the
> pygrub path, this would end up setting up a new tap device.
>
> 3) There is no tap-specific teardown code on the pygrub path, and I
> don't want to add any (particularly since I'm hoping to remove tapdisk
> altogether soon).
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Changes since v1:
> - Re-port on top of revisions to previous patches
> - Address line length issues
> - Remove stray space at the end of an if (xxx )
> - Break out devicemodel argument rearrangement into a separate patch
> - Spell out implications of warning messages which skip emulated disks
>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Anthony Perard <anthony.perard@citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> ---
>  tools/libxl/libxl.c          | 17 ++++++++++++++---
>  tools/libxl/libxl_dm.c       | 27 +++++++++++++++++++++++----
>  tools/libxl/libxl_internal.h |  8 ++++++++
>  3 files changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2d7a154..602bec2 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3000,8 +3000,9 @@ static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user,
>
>  /* Callbacks */
>
> -static char *libxl__device_disk_find_local_path(libxl__gc *gc,
> -                                                const libxl_device_disk *disk)
> +char *libxl__device_disk_find_local_path(libxl__gc *gc,
> +                                         const libxl_device_disk *disk,
> +                                         bool qdisk_direct)
>  {
>      char *path = NULL;
>
> @@ -3022,6 +3023,16 @@ static char *libxl__device_disk_find_local_path(libxl__gc *gc,
>          goto out;
>      }
>
> +    /*
> +     * If we're being called for a qemu path, we can pass the target
> +     * string directly as well
> +     */
> +    if (qdisk_direct && disk->backend == LIBXL_DISK_BACKEND_QDISK) {
> +        path = libxl__strdup(gc, disk->pdev_path);
> +        LOG(DEBUG, "Directly accessing local QDISK target %s", path);
> +        goto out;
> +    }
> +
>   out:
>      return path;
>  }
> @@ -3046,7 +3057,7 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
>
>      LOG(DEBUG, "Trying to find local path");
>
> -    dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk);
> +    dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk, false);
>      if (dls->diskpath) {
>          LOG(DEBUG, "Local path found, executing callback.");
>          dls->callback(egc, dls, 0);
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index dfcf141..1493b84 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1182,23 +1182,42 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>
>              if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
>                  if (!disks[i].is_cdrom) {
> -                    LOG(WARN, "cannot support"" empty disk format for %s",
> +                    LOG(WARN, "Cannot support empty disk format for %s",
>                          disks[i].vdev);
>                      continue;
>                  }
>              } else {
>                  if (format == NULL) {
>                      LOG(WARN,
> -                        "unable to determine"" disk image format %s",
> +                        "Unable to determine disk image format: %s\n",
> +                        "Disk will be available via PV drivers but not as an"
> +                        "emulated disk.",

Sorry, there's a stray comma after the first line of the multi-line
string.  Let me know if you want me to re-send the series with the
comma removed, or whether someone can take it out when committing.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel