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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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;
  }

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

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

Ian.

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