| Message ID | 1458144557-29070-7-git-send-email-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
George Dunlap writes ("[PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts"):
> pygrub and qemuu need to be able to access a VM's disks locally in
> order to be able to pull out the kernel and provide emulated disk
> access, respectively. This can be done either by accessing the local
> disk directly, or by plugging the target disk into dom0 to allow
> access.
Some long lines, `if(', etc., in this patch, I'm afraid.
> Unfortunately, while the plugging machinery works for pygrub, it does
> not yet work for qemuu; meaning that disk hotplug scripts cannot be
> used with HVM domains.
>
> Fortunately, disks using hotplug scripts created in dom0 do create a
> block device as part of set-up, which can be accessed locally; and if
> they use block-common.sh:write_dev, this path will bre written to
> physical-device-path.
>
> Modify libxl__device_disk_setdefault() to be able to fish this path
> out of xenstore and pass it back.
>
> We need the target domid to find the appropriate xenstore node, so add
> that to libxl__disk_local_state.
I couldn't find that change in this patch.
> + /*
> + * If the format isn't raw and / or we're using a script, then see
> + * if the script has written a path to the "cooked" node
> + */
> + if(disk->script && domid != INVALID_DOMID) {
> + libxl__device device;
> + char *be_path, *pdpath;
> + int rc;
I don't see where you check that the disk is not being provided by a
driver domain - in which case the hotplug script ran in the driver
domain and the device path is also only in the driver domain.
(By `driver domain' I mean, really, a domain other than this one.)
> _hidden char * libxl__device_disk_find_local_path(libxl__gc *gc,
> + libxl_domid domid,
This new parameter would maybe be a little less confusing if it were
explicitly `guest_domid'; after all it might be the driver domain
domid.
Thanks,
Ian.
On Thu, Mar 17, 2016 at 6:36 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > George Dunlap writes ("[PATCH 6/8] libxl: Allow local access for block devices with hotplug scripts"): >> pygrub and qemuu need to be able to access a VM's disks locally in >> order to be able to pull out the kernel and provide emulated disk >> access, respectively. This can be done either by accessing the local >> disk directly, or by plugging the target disk into dom0 to allow >> access. > > Some long lines, `if(', etc., in this patch, I'm afraid. > >> Unfortunately, while the plugging machinery works for pygrub, it does >> not yet work for qemuu; meaning that disk hotplug scripts cannot be >> used with HVM domains. >> >> Fortunately, disks using hotplug scripts created in dom0 do create a >> block device as part of set-up, which can be accessed locally; and if >> they use block-common.sh:write_dev, this path will bre written to >> physical-device-path. >> >> Modify libxl__device_disk_setdefault() to be able to fish this path >> out of xenstore and pass it back. >> >> We need the target domid to find the appropriate xenstore node, so add >> that to libxl__disk_local_state. > > I couldn't find that change in this patch. Oops -- forgot to take this paragraph out after porting it on top of patch 2 (which changes libxl not to duplicate the work of 'block' anymore). I'll remove this. > >> + /* >> + * If the format isn't raw and / or we're using a script, then see >> + * if the script has written a path to the "cooked" node >> + */ >> + if(disk->script && domid != INVALID_DOMID) { >> + libxl__device device; >> + char *be_path, *pdpath; >> + int rc; > > I don't see where you check that the disk is not being provided by a > driver domain - in which case the hotplug script ran in the driver > domain and the device path is also only in the driver domain. > > (By `driver domain' I mean, really, a domain other than this one.) It's in patch 4 (still at the top of this function): + /* No local paths for driver domains */ + if (disk->backend_domname != NULL) { + LOG(DEBUG, "Non-local backend, can't access locally.\n"); + goto out; + } > >> _hidden char * libxl__device_disk_find_local_path(libxl__gc *gc, >> + libxl_domid domid, > > This new parameter would maybe be a little less confusing if it were > explicitly `guest_domid'; after all it might be the driver domain > domid. Ack. -George
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index d6302a9..490a7d2 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -2296,7 +2296,7 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk) } int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, - libxl_device_disk *disk, + const libxl_device_disk *disk, libxl__device *device) { int devid; @@ -3002,6 +3002,7 @@ static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user, /* Callbacks */ char * libxl__device_disk_find_local_path(libxl__gc *gc, + libxl_domid domid, const libxl_device_disk *disk, bool qdisk_direct) { char *path = NULL; @@ -3032,6 +3033,37 @@ char * libxl__device_disk_find_local_path(libxl__gc *gc, path = libxl__strdup(gc, disk->pdev_path); LOG(DEBUG, "Directly accessing local QDISK target %s", path); goto out; + } + + /* + * If the format isn't raw and / or we're using a script, then see + * if the script has written a path to the "cooked" node + */ + if(disk->script && domid != INVALID_DOMID) { + libxl__device device; + char *be_path, *pdpath; + int rc; + + LOG(DEBUG, "Run from a script; checking for physical-device-path (vdev %s)", + disk->vdev); + + rc = libxl__device_from_disk(gc, domid, disk, &device); + if (rc < 0) + goto out; + + be_path = libxl__device_backend_path(gc, &device); + + pdpath = libxl__sprintf(gc, "%s/physical-device-path", be_path); + + LOG(DEBUG, "Attempting to read node %s", pdpath); + path = libxl__xs_read(gc, XBT_NULL, pdpath); + + if (path) + LOG(DEBUG, "Accessing cooked block device %s", path); + else + LOG(DEBUG, "No physical-device-path, can't access locally."); + + goto out; } out: @@ -3058,7 +3090,8 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc, LOG(DEBUG, "Trying to find local path"); - if ((dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk, false))) { + if ((dls->diskpath = libxl__device_disk_find_local_path(gc, INVALID_DOMID, + in_disk, false))) { LOG(DEBUG, "Local path found, executing callback."); dls->callback(egc, dls, 0); } else { @@ -3073,7 +3106,6 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc, rc = libxl__device_disk_setdefault(gc, disk); if (rc) goto out; - /* If we can't find a local path, attach it */ libxl__prepare_ao_device(ao, &dls->aodev); dls->aodev.callback = local_device_attach_cb; device_disk_add(egc, LIBXL_TOOLSTACK_DOMID, disk, &dls->aodev, diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 917ebbf..646a49c 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -1206,8 +1206,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, target_path = libxl__blktap_devpath(gc, disks[i].pdev_path, disks[i].format); else - target_path = libxl__device_disk_find_local_path(gc, - &disks[i], true); + target_path = libxl__device_disk_find_local_path(gc, guest_domid, &disks[i], true); if (!target_path) { LOG(WARN, "No way to get local access disk to image: %s", disks[i].vdev); diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index a511c6d..6291b9c 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1709,7 +1709,7 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc, _hidden int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params); _hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, - libxl_device_disk *disk, + const libxl_device_disk *disk, libxl__device *device); /* Calls poll() again - useful to check whether a signaled condition @@ -2674,6 +2674,7 @@ static inline void libxl__device_disk_local_init(libxl__disk_local_state *dls) * See if we can find a way to access a disk locally */ _hidden char * libxl__device_disk_find_local_path(libxl__gc *gc, + libxl_domid domid, const libxl_device_disk *disk, bool qdisk_direct);