Re: [Qemu-devel] [PATCH v3 3/3] xen: perform XenDevice clean-up in XenBus watch handler

[Anthony PERARD]

On Fri, Sep 13, 2019 at 09:21:58AM +0100, Paul Durrant wrote:
> Cleaning up offline XenDevice objects directly in
> xen_device_backend_changed() is dangerous as xen_device_unrealize() will
> modify the watch list that is being walked. Even the QLIST_FOREACH_SAFE()
> used in notifier_list_notify() is insufficient as *two* notifiers (for
> the frontend and backend watches) are removed, thus potentially rendering
> the 'next' pointer unsafe.
> 
> The solution is to use the XenBus backend_watch handler to do the clean-up
> instead, as it is invoked whilst walking a separate watch list.
> 
> This patch therefore adds a new 'inactive_devices' list to XenBus, to which
> offline devices are added by xen_device_backend_changed(). The XenBus
> backend_watch registration is also changed to not only invoke
> xen_bus_enumerate() but also a new xen_bus_cleanup() function, which will
> walk 'inactive_devices' and perform the necessary actions.
> For safety an extra 'online' check is also added to xen_bus_type_enumerate()
> to make sure that no attempt is made to create a new XenDevice object for a
> backend that is offline.
> 
> NOTE: This patch also includes some cosmetic changes:
>       - substitute the local variable name 'backend_state'
>         in xen_bus_type_enumerate() with 'state', since there
>         is no ambiguity with any other state in that context.
>       - change xen_device_state_is_active() to
>         xen_device_frontend_is_active() (and pass a XenDevice directly)
>         since the state tests contained therein only apply to a frontend.
>       - use 'state' rather then 'xendev->backend_state' in
>         xen_device_backend_changed() to shorten the code.
> 
> Signed-off-by: Paul Durrant <address@hidden>
> ---
> Cc: Stefano Stabellini <address@hidden>
> Cc: Anthony Perard <address@hidden>
> 
> v3:
>  - s/offline_devices/inactive_devices/g
>  - Also add an 'inactive' boolean to XenDevice which is set when the
>    device is added to the inactive list so we really can make sure that it
>    doesn't happen more than once

Reviewed-by: Anthony PERARD <address@hidden>

Thanks,

-- 
Anthony PERARD