Re: [Xen-devel] [PATCH v19 04/12] libxl/remus: introduce an abstract Remus device layer

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Yang Hongyang writes ("[PATCH v19 04/12] libxl/remus: introduce an abstract 
Remus device layer"):
> Introduce an abstract device layer that allows the Remus
> logic in libxl to control a guest's devices in a device-agnostic
> manner. The device layer also exposes a set of internal interfaces
> that a device type must implement, if it wishes to support Remus.
...


I went to double-check this and found that I hadn't looked at your
revised searching-for-an-appropriate-subkind code.

> diff --git a/tools/libxl/libxl_remus_device.c 
> b/tools/libxl/libxl_remus_device.c
> new file mode 100644
> index 0000000..835d095
> --- /dev/null
> +++ b/tools/libxl/libxl_remus_device.c
...
> +static void remus_devices_setup(libxl__egc *egc,
> +                                libxl__remus_devices_state *rds)
> +{
> +    int i, rc;
> +    libxl__remus_device *dev;
> +
> +    STATE_AO_GC(rds->ao);
> +
> +    libxl__multidev_begin(ao, &rds->multidev);
> +    rds->multidev.callback = devices_setup_cb;
...
> +        libxl__multidev_prepare_with_aodev(&rds->multidev, &dev->aodev);
...
> +static void devices_setup_cb(libxl__egc *egc,
> +                             libxl__multidev *multidev,
> +                             int rc)
...
> +    if (rc == ERROR_REMUS_DEVOPS_DOES_NOT_MATCH) {
> +        remus_devices_setup(egc, rds);

This improperly reenters the multidev.  See the doc comment for
multidev.  You mustn't call libxl__multidev_begin until the whole
thing is done.

The cause of this is that the loop is still somewhat inside-out.  You
need to call the aodev's callback only when you have found the right
device.

Perhaps I should try to rewrite this part.


> +struct libxl__remus_devices_state {
...
> +    libxl__egc *egc;

I have just spotted this field.  I think this is quite wrong.

As the comment in libxl_internal.h has it:
     * The egc and its gc may be accessed only on the creating thread. */

And of course the event callbacks which your code receives may occur
on any thread.


> diff --git a/tools/libxl/libxl_types_internal.idl 
> b/tools/libxl/libxl_types_internal.idl
> index 800361b..720232e 100644
> --- a/tools/libxl/libxl_types_internal.idl
> +++ b/tools/libxl/libxl_types_internal.idl
> @@ -22,6 +22,8 @@ libxl__device_kind = Enumeration("device_kind", [
>      (6, "VKBD"),
>      (7, "CONSOLE"),
>      (8, "VTPM"),
> +    (9, "REMUS_NIC"),
> +    (10, "REMUS_DISK"),

I don't understand why these need new enum values.  What's wrong with
VIF and VBD ?


Ian.

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