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

[Marek Marczykowski <marmarek@xxxxxxxxxxxx>]

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)?

-- 
Pozdrawiam / Best Regards,
Marek Marczykowski         | RLU #390519
marmarek at mimuw edu pl   | xmpp:marmarek at staszic waw pl

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel