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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel