Re: [PATCH 01 of 10] libxl: Remove frontend and backend devices from xenstore after destroy

[Ian Campbell <Ian.Campbell@eu.citrix.com>]

On Sat, 2011-06-04 at 00:30 +0100, Marek Marczykowski wrote:
> On 03.06.2011 10:03, Ian Campbell wrote:
> > Hi Marek,
> > 
> > Please can you add:
> > [diff]
> > showfunc = True
> > to your ~/.hgrc. It makes patches much easier to read.
> > 
> >  On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> >> # HG changeset patch
> >> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> >> # Date 1306962865 -7200
> >> # Node ID e3a3f5cc95349e92b7cb8b1448e999ffc16bd060
> >> # Parent  43acc031eb24945973dffda2b7caf976993bbd5f
> >> libxl: Remove frontend and backend devices from xenstore after destroy
> >>
> >> Cleanup frontend and backend devices from xenstore for all dev types - not only
> >> disks.
> >>
> >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> > 
> > Thanks!
> > 
> > The whole device destroy/teardown path is a bit of a twisty maze of
> > confusingly named functions and parameters (e.g. force == !wait, destroy
> > vs delete vs remove etc). Any attempt to try and untangle things is most
> > welcome and I think this is a step in the right direction.
> > 
> >>
> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >> --- a/tools/libxl/libxl.c
> >> +++ b/tools/libxl/libxl.c
> >> @@ -1065,8 +1065,6 @@
> >>      device.devid            = devid;
> >>      device.kind             = DEVICE_VBD;
> >>      rc = libxl__device_del(ctx, &device, wait);
> >> -    xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, &device));
> >> -    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, &device));
> >>      libxl__free_all(&gc);
> >>      return rc;
> >>  }
> >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> >> --- a/tools/libxl/libxl_device.c
> >> +++ b/tools/libxl/libxl_device.c
> >> @@ -401,6 +401,8 @@
> >>          (void)wait_for_dev_destroy(ctx, &tv);
> >>      }
> >>  
> >> +    xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, dev));
> >> +    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev));
> > 
> > In the case where wait == true libxl__device_destroy will add a watch on
> > the be path and wait_for_dev_destroy will subsequently remove the path.
> 
> Only backend one...
> 
> > I think it would be better to add the xs_rm for be path to
> > libxl__device_destroy as the else clause to the !force which adds the
> > watch? 
> 
> Also - only backend one (libxl__device_destroy gets only backend path as
> parameter).
> 
> > I think this allow the other caller (libxl__devices_destroy) to
> > be simplified too since all that mucking around with the toremove
> > flexarray could be dropped.
> 
> What about frontend device in xenstore? In most cases only backend is
> removed. In wait_for_dev_destroy we don't know frontend patch (wheel,
> not exactly true, but not always easy). Perhaps there should be two
> watches - on backend AND on frontend (with writing "5" to frontend state
> also?)? Or backend should be removed as you suggested, but frontend as
> in my patch (after wait_for_dev_destroy if was called)?

My comments were only about the location of the rm of the backend path.

I think your original patch moved the rm of the frontend path to the
right place.

an.