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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 20 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v3 6/7] xl/libxl: 
> implement QDISK libxl_device_disk_local_attach"):
> > 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.
> 
> No, I mean that the variable should be defined at the top of the
> function and initialised to 0.
> 
> In general, any resource allocated in a function other than from the
> gc should be held in a variable set and used like "resource" in this
> example:
> 
>   int libxl__do_something_requiring_xmumble_resource(libxl__gc *gc, flibble)
>   {
> 
>       foo *resource = 0;
>       int rc;
> 
>       blah blah blah;
> 
>       resource = xmumble_acquire_resource(wibble wombat);
>       if (!resource) { rc = ERROR_FAIL; goto out; }
> 
>       blah blah blah;
> 
>       rc = 0;
> 
>     out:
>       xmumble_free_resource(resource);
>       return rc;
>   }

After reading the rest of this email, I can see why this would be a
useful change to make.


> > > > +                    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.
> 
> No, I mean only the calls which abort the transaction should be in the
> "goto out" section.
> 
> So:
> 
>   int libxl__do_something_requiring_xmumble_resource(libxl__gc *gc, flibble)
>   {
> 
>       xs_transaction_t our_trans;
>       int rc;
> 
>       blah blah blah;
> 
>       for (;;) {
>           our_trans = xs_transaction_start(...);
>           if (!our_trans) { rc = ERROR_FAIL; goto out; }
> 
>           blah blah blah;
> 
>           r = xs_transaction_end(CTX->xsh, our_trans, 0);
>           our_trans = 0; /* it's ended either way */
>           if (r shows it was successful) break;
>           if (r not as expected) { rc = ERROR_FAIL; goto out; }
>       }
> 
>       blah blah blah;
> 
>       rc = 0;
> 
>     out:
>       if (our_trans) xs_transaction_end(CTX->xsh, our_trans, 1);
>       return rc;
>   }
> 
> The point is that the following invariant is maintained: we have a
> transaction open, which needs to be ended, iff !!our_trans.

OK


> > > > -            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.
> 
> I'm not sure I follow.  This is the attachment of the VM's disk for
> the benefit of the bootloader.  When we are done with running the
> bootloader we need to clean it up.
 
Sorry I didn't understand what you meant earlier.
I'll use another why to figure out if we need to remove the disk
that is not dependent on the format or the policy that we used earlier.

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