Re: [Xen-devel] [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Stefano Stabellini writes ("[Xen-devel] [PATCH v3 6/7] xl/libxl: implement 
QDISK libxl_device_disk_local_attach"):
...
> +                    t = xs_transaction_start(ctx->xsh);

This transaction should be in the local variables block for the whole
function, and initialised to 0.

> +                    /* use 0 as the domid of the toolstack domain for now */
> +                    tmpdisk->vdev = libxl__alloc_vdev(gc, 0, blkdev_start, 
> t);

Should this 0 be abstracted into a #define or a variable ?

> +                    if (tmpdisk->vdev == NULL) {
> +                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                                "libxl__alloc_vdev failed\n");
> +                        xs_transaction_end(ctx->xsh, t, 1);

And then all these xs_transaction_end calls turn into one call in the
exit path.

> +                dev = libxl__sprintf(gc, "/dev/%s", tmpdisk->vdev);

Perhaps you'd like to use GCSPRINTF now that we have it.

>              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> -                       disk->pdev_path);

Perhaps you'd like to use LOG(DEBUG, ....") now that we have it ?

> -            dev = tmpdisk->pdev_path;
> +    switch (disk->backend) {
> +        case LIBXL_DISK_BACKEND_QDISK:
> +            if (disk->format != LIBXL_DISK_FORMAT_RAW) {

This replicates the logic earlier, which decided whether to use a
qdisk.  I think it would be better to remember whether we did use a
qdisk and detach it iff so.

Ian.

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