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@xxxxxxxxxxxx>
> >> # 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@xxxxxxxxxxxx>
> >
> > 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home