Re: [Xen-devel] [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH 5/8] libxl: Share logic for 
finding path between qemuu and pygrub"):
> That's what the "qdisk_direct" argument is for -- it tells
> libxl__device_disk_find_local_path, "I can interpret QDISK backends".
> Pygrub will call this function with "qdisk_direct" set to "false",
> since it can't interpret QDISK backends; this will result in it doing
> a local attach instead.

Thanks for the explanation.

Acked-by: Ian Jackson <ian.jack...@eu.citrix.com>

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

Re: [Xen-devel] [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub

[George Dunlap]

On Thu, Mar 17, 2016 at 6:22 PM, Ian Jackson  wrote:
> George Dunlap writes ("[PATCH 5/8] libxl: Share logic for finding path 
> between qemuu and pygrub"):
>> From: George Dunlap 
>
> Thanks.  There are a few format inconsistencies (long lines, spaces)
> which I won't enumerate.
>
>> * In the case of a block script, or a non-dom0 backend, qemuu will now
>> print a warning and skip the disk, rather than adding bogus
>> parameters that will cause qemuu to error out.
>
> I think the consequences here ought to be better spelled out.  AFAICT
> the implication is that, in this case, the disk will be available to
> the guest as a PV disk (because libxl will select a backend other than
> qdisk) but not via the emulated IDE ?
>
> IWBNI the resulting functional restriction were documented, but I
> won't insist on that given that you're improving matters.
>
>> @@ -3023,6 +3024,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;
>
> Is this really true ?  What if the format is qcow2 ?  I think that
> might result in feeding pygrub the qcow2 file rather than the virtual
> block image it contains.

That's what the "qdisk_direct" argument is for -- it tells
libxl__device_disk_find_local_path, "I can interpret QDISK backends".
Pygrub will call this function with "qdisk_direct" set to "false",
since it can't interpret QDISK backends; this will result in it doing
a local attach instead.

> Overall I think this patch is otherwise probably good but it it's a
> bit hard to see the wood for the trees.  If you felt like factoring
> out some of the refactoring and variable renaming, from the functional
> change, that would be very nice.

Yes, there are a lot of things going on, aren't there.  I think I
remember trying to make it simpler when I first wrote it last year,
but not finding any really sensible in-between points; but I'll give
it another look.

 -George

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

[Xen-devel] [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub

[George Dunlap]

From: George Dunlap 

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.

Reorganize the qemuu disk argument code to make a clean separation
between finding a file to use (if any), and constructing the
parameters.

Only use qemu_disk_format_string() in circumstances where qemu may be
interpreting the disk (i.e., backend==QDISK).  In all other cases, it
should use RAW.

Share as much as possible between the is_cdrom path and the normal path.

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.

This should fix two bugs:

* In the case of a block script, or a non-dom0 backend, qemuu will now
print a warning and skip the disk, rather than adding bogus
parameters that will cause qemuu to error out.

* You should now be able to use any backend for a cdrom that you can
use for a normal disk.  (Before it was limited to RAW files or things
that qemu could handle directly.)

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 
---
CC: Ian Jackson 
CC: Wei Liu 
CC: Stefano Stabellini 
CC: Anthony Perard 
CC: Roger Pau Monne 
---
 tools/libxl/libxl.c  | 17 ++--
 tools/libxl/libxl_dm.c   | 66 +---
 tools/libxl/libxl_internal.h |  8 ++
 3 files changed, 66 insertions(+), 25 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a57568f..d6302a9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3001,8 +3001,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;
 
 /* No local paths for driver domains */
@@ -3023,6 +3024,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;
 }
@@ -3047,7 +3058,7 @@ 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))) {
+if ((dls->diskpath = libxl__device_disk_find_local_path(gc, in_disk, 
false))) {
 LOG(DEBUG, "Local path found, executing callback.");
 dls->callback(egc, dls, 0);
 } else {
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4aca38e..917ebbf 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1161,9 +1161,9 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
 int disk, part;
 int dev_number =
 libxl__device_disk_dev_number(disks[i].vdev, , );
-const char *format = qemu_disk_format_string(disks[i].format);
+const char *format;
 char *drive;
-const char *pdev_path;
+const char *target_path;
 
 if (dev_number == -1) {
 LOG(WARN, "unable to determine"" disk number for %s",
@@ -1171,22 +1171,22 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
 continue;
 }
 
-if (disks[i].is_cdrom) {
-if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
-drive = libxl__sprintf
-(gc, 
"if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
- disk, disks[i].readwrite ? "off" : "on", dev_number);
-else
-drive = libxl__sprintf
-(gc, 

Re: [Xen-devel] [PATCH 5/8] libxl: Share logic for finding path between qemuu and pygrub

[Ian Jackson]

George Dunlap writes ("[PATCH 5/8] libxl: Share logic for finding path between 
qemuu and pygrub"):
> From: George Dunlap 

Thanks.  There are a few format inconsistencies (long lines, spaces)
which I won't enumerate.

> * In the case of a block script, or a non-dom0 backend, qemuu will now
> print a warning and skip the disk, rather than adding bogus
> parameters that will cause qemuu to error out.

I think the consequences here ought to be better spelled out.  AFAICT
the implication is that, in this case, the disk will be available to
the guest as a PV disk (because libxl will select a backend other than
qdisk) but not via the emulated IDE ?

IWBNI the resulting functional restriction were documented, but I
won't insist on that given that you're improving matters.

> @@ -3023,6 +3024,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;

Is this really true ?  What if the format is qcow2 ?  I think that
might result in feeding pygrub the qcow2 file rather than the virtual
block image it contains.

Overall I think this patch is otherwise probably good but it it's a
bit hard to see the wood for the trees.  If you felt like factoring
out some of the refactoring and variable renaming, from the functional
change, that would be very nice.

Ian.

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