Re: [Xen-devel] [PATCH v2 1/2] libxl/devd: fix a race with concurrent device addition/removal

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v2 1/2] libxl/devd: fix a race with concurrent 
device addition/removal"):
> Current code can free the libxl__device inside of the libxl__ddomain_device
> before the addition has finished if a removal happens while an addition is
> still in process:
...
> Fix this by creating a temporary copy of the libxl__device, that's
> tracked by the GC of the nested async operation. This ensures that
> the libxl__device used by the async operations cannot be freed while
> being used.
...
>          GCNEW(aodev);
>          libxl__prepare_ao_device(ao, aodev);
> -        aodev->dev = dev;
> +        /*
> +         * Clone the libxl__device to avoid races if remove_device is called
> +         * before the device addition has finished.
> +         */
> +        GCNEW(aodev->dev);
> +        *aodev->dev = *dev;

This does conveniently disentangle the memory management, so I think
it's a good approach.

But it reads kind of oddly to me.  I think it is not buggy, but can
you add a comment to the definition of libxl__device, saying that it
is a transparent structure containing no external memory references ?

Otherwise this copy is not really justifiable, because in C, in
general, structs might contain private fields, or memory references or
linked list entries or something.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel