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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

Could you please avoid to remove so much context from your quotes? It
makes it very hard for me to read your emails.


On Tue, 17 Apr 2012, Ian Jackson wrote:
> 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.

I don't think I understand what you mean.
Do you mean moving xs_transaction_start outside the switch?
If so, why? It doesn't affect many of the other cases.


> > +                    /* 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 ?

Yes, good idea.


> > +                    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.

So maybe you do mean moving the transaction outside the switch.
In that case I disagree: the transaction is only useful in a very
limited set of cases (just one at the moment), while most of the others
don't need it.


> > +                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.
 
There are cases in which we do use qdisk but we don't have to detach.

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