Re: [Xen-devel] [PATCH] libxl: do not fail device removal if backend domain is gone

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Feb 09, 2018 at 11:27:04AM +0000, Roger Pau Monné wrote:
> On Fri, Feb 09, 2018 at 12:22:13AM +0100, Marek Marczykowski-Górecki wrote:
> > Backend domain may be independently destroyed - there is no
> > synchronization of libxl structures (including /libxl tree) elsewhere.
> > Backend might also remove the device info from its backend xenstore
> > subtree on its own.
> > If such situation is detected, do not fail the removal, but finish the
> > cleanup of the frontend side.
> > 
> > This is just workaround, the real fix should watch when the device
> > backend is removed (including backend domain destruction) and remove
> > frontend at that time. And report such event to higher layer code, so
> > for example libvirt could synchronize its state.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  tools/libxl/libxl_device.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > index 1b796bd392..1f58a99a23 100644
> > --- a/tools/libxl/libxl_device.c
> > +++ b/tools/libxl/libxl_device.c
> > @@ -997,6 +997,13 @@ void libxl__initiate_device_generic_remove(libxl__egc 
> > *egc,
> >              goto out;
> >          }
> >  
> > +        /* if state_path is empty, assume backend is gone (backend domain
> > +         * shutdown?), cleanup frontend only; rc=0 */
> > +        if (!state) {
> > +            LOG(WARN, "backend %s already removed, cleanup frontend only", 
> > be_path);
> > +            goto out;
> > +        }
> > +
> 
> I think INFO should be used instead of WARN, since this doesn't look
> to be a cause for concern from an admin PoV.

Ok, will change.

> I'm also wondering, if you jump to 'out' here, you avoid the call to
> libxl__xs_transaction_commit and instead end up calling
> libxl__xs_transaction_abort, which means the above call to
> libxl__xs_path_cleanup will not be committed to xenstore, is this
> really desired?
>
> It seems to me libxl might leak xenstore frontend entries in that
> case.

That call is only if aodev->force. In other cases cleanup is done in
device_hotplug_done()->libxl__device_destroy(), which have its own transaction.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel