Re: [PATCH v3 16/33] tools/libs/light: add backend type for 9pfs PV devices

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Jan 04, 2024 at 10:00:38AM +0100, Juergen Gross wrote:
> diff --git a/tools/libs/light/libxl_9pfs.c b/tools/libs/light/libxl_9pfs.c
> index 5ab0d3aa21..486bc4326e 100644
> --- a/tools/libs/light/libxl_9pfs.c
> +++ b/tools/libs/light/libxl_9pfs.c
> @@ -33,20 +33,159 @@ static int libxl__set_xenstore_p9(libxl__gc *gc, 
> uint32_t domid,
>  
>      flexarray_append_pair(front, "tag", p9->tag);
>  
> +    if (p9->type == LIBXL_P9_TYPE_XEN_9PFSD) {
> +        flexarray_append_pair(back, "max-space",
> +                              GCSPRINTF("%u", p9->max_space));
> +        flexarray_append_pair(back, "max-files",
> +                              GCSPRINTF("%u", p9->max_files));
> +        flexarray_append_pair(back, "max-open-files",
> +                              GCSPRINTF("%u", p9->max_open_files));
> +        flexarray_append_pair(back, "auto-delete",
> +                              p9->auto_delete ? "1" : "0");
> +    }
> +
> +    return 0;
> +}
> +
> +static int libxl__device_from_p9(libxl__gc *gc, uint32_t domid,
> +                                 libxl_device_p9 *type, libxl__device 
> *device)
> +{
> +    device->backend_devid   = type->devid;
> +    device->backend_domid   = type->backend_domid;
> +    device->backend_kind    = type->type == LIBXL_P9_TYPE_QEMU
> +                              ? LIBXL__DEVICE_KIND_9PFS
> +                              : LIBXL__DEVICE_KIND_XEN_9PFS;
> +    device->devid           = type->devid;
> +    device->domid           = domid;
> +    device->kind            = LIBXL__DEVICE_KIND_9PFS;
> +
>      return 0;
>  }
>  
> -#define libxl__add_p9s NULL
> +static int libxl_device_p9_dm_needed(void *e, unsigned domid)

Prefix of the function should be "libxl__" as it's only internal to
libxl.

> +{
> +    libxl_device_p9 *elem = e;
> +
> +    return elem->type == LIBXL_P9_TYPE_QEMU && elem->backend_domid == domid;
> +}
> +
> +typedef struct libxl__aop9_state libxl__aop9_state;
> +
> +struct libxl__aop9_state {
> +    libxl__spawn_state spawn;
> +    libxl__ao_device *aodev;
> +    libxl_device_p9 *p9;
> +    uint32_t domid;
> +    void (*callback)(libxl__egc *, libxl__aop9_state *, int);
> +};
> +
> +static void xen9pfsd_spawn_outcome(libxl__egc *egc, libxl__aop9_state *aop9,
> +                                   int rc)
> +{
> +    aop9->aodev->rc = rc;
> +    if (rc)
> +        aop9->aodev->callback(egc, aop9->aodev);
> +    else
> +        libxl__device_add_async(egc, aop9->domid, &libxl__p9_devtype,
> +                                aop9->p9, aop9->aodev);
> +}
> +
> +static void xen9pfsd_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
> +                             const char *xsdata)
> +{
> +    STATE_AO_GC(spawn->ao);
> +
> +    if (!xsdata)
> +        return;
> +
> +    if (strcmp(xsdata, "running"))
> +        return;
> +
> +    libxl__spawn_initiate_detach(gc, spawn);
> +}
> +
> +static void xen9pfsd_failed(libxl__egc *egc, libxl__spawn_state *spawn, int 
> rc)
> +{
> +    libxl__aop9_state *aop9 = CONTAINER_OF(spawn, *aop9, spawn);
> +
> +    xen9pfsd_spawn_outcome(egc, aop9, rc);
> +}
> +
> +static void xen9pfsd_detached(libxl__egc *egc, libxl__spawn_state *spawn)
> +{
> +    libxl__aop9_state *aop9 = CONTAINER_OF(spawn, *aop9, spawn);
> +
> +    xen9pfsd_spawn_outcome(egc, aop9, 0);
> +}
> +
> +static int xen9pfsd_spawn(libxl__egc *egc, uint32_t domid, libxl_device_p9 
> *p9,
> +                         libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    struct libxl__aop9_state *aop9;
> +    int rc;
> +    char *args[] = { "xen-9pfsd", NULL };
> +    char *path = GCSPRINTF("/local/domain/%u/libxl/xen-9pfs",
> +                           p9->backend_domid);
> +
> +    if (p9->type != LIBXL_P9_TYPE_XEN_9PFSD ||
> +        libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/state", path)))

I feel like this check and this function might not work as expected.
What happen if we try to add more than one 9pfs "device"? libxl I think
is going to try to start several xen-9pfs daemon before the first one
have had time to write the "*/state" path.
What about two different libxl process trying to spawn that daemon? Is
xen-9pfs going to behave well and have one giveup? But that would
probably mean that libxl is going to have an error due to the process
exiting early, maybe.

> +        return 0;
> +
> +    GCNEW(aop9);
> +    aop9->aodev = aodev;
> +    aop9->p9 = p9;
> +    aop9->domid = domid;
> +    aop9->callback = xen9pfsd_spawn_outcome;
> +
> +    aop9->spawn.ao = aodev->ao;
> +    aop9->spawn.what = "xen-9pfs daemon";
> +    aop9->spawn.xspath = GCSPRINTF("%s/state", path);
> +    aop9->spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
> +    aop9->spawn.pidpath = GCSPRINTF("%s/pid", path);
> +    aop9->spawn.midproc_cb = libxl__spawn_record_pid;
> +    aop9->spawn.confirm_cb = xen9pfsd_confirm;
> +    aop9->spawn.failure_cb = xen9pfsd_failed;
> +    aop9->spawn.detached_cb = xen9pfsd_detached;
> +    rc = libxl__spawn_spawn(egc, &aop9->spawn);
> +    if (rc < 0)
> +        return rc;
> +    if (!rc) {
> +        setsid();
> +        libxl__exec(gc, -1, -1, -1, LIBEXEC_BIN "/xen-9pfsd", args, NULL);
> +    }
> +
> +    return 1;
> +}

Could you reorder the file, to make it easier to follow the code of
the async style? "xen9pfsd_spawn()" should be first, followed by
_confirm() _failed and _detached() and finally xen9pfsd_spawn_outcome().

> +
> +static void libxl__device_p9_add(libxl__egc *egc, uint32_t domid,
> +                                 libxl_device_p9 *p9,
> +                                 libxl__ao_device *aodev)
> +{
> +    int rc;
> +
> +    rc = xen9pfsd_spawn(egc, domid, p9, aodev);
> +    if (rc == 1)

I'd like a comment about what's different about rc==1 vs rc==0, here or
in the function xen9pfsd_spawn. These functions that sometime setup a
callback (or actually call it) and sometime don't, make things harder to
follow.

Or maybe we could rewrite things a bit so that there's only one function
that calls libxl__device_add_async(). But the current
libxl__device_p9_add() is kind of ok, with additional comments.

> +        return;
> +
> +    if (rc == 0)
> +        libxl__device_add_async(egc, domid, &libxl__p9_devtype, p9, aodev);
> +
> +    aodev->rc = rc;
> +    if (rc)
> +        aodev->callback(egc, aodev);
> +}
> +
>  #define libxl_device_p9_list NULL
>  #define libxl_device_p9_compare NULL
>  
>  static LIBXL_DEFINE_UPDATE_DEVID(p9)
> -static LIBXL_DEFINE_DEVICE_FROM_TYPE(p9)
> +static LIBXL_DEFINE_DEVICES_ADD(p9)
>  
>  LIBXL_DEFINE_DEVICE_REMOVE(p9)
>  
>  DEFINE_DEVICE_TYPE_STRUCT(p9, 9PFS, p9s,
> -    .skip_attach = 1,
>      .set_xenstore_config = (device_set_xenstore_config_fn_t)
>                             libxl__set_xenstore_p9,
> +    .dm_needed = libxl_device_p9_dm_needed,
>  );
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index 7d8bd5d216..82565c4c10 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -150,6 +150,12 @@ libxl_nic_type = Enumeration("nic_type", [
>      (2, "VIF"),
>      ])
>  
> +libxl_p9_type = Enumeration("p9_type", [
> +    (0, "unknown"),
> +    (1, "qemu"),
> +    (2, "xen_9pfsd"),
> +    ])
> +
>  libxl_action_on_shutdown = Enumeration("action_on_shutdown", [
>      (1, "DESTROY"),
>  
> @@ -942,6 +948,11 @@ libxl_device_p9 = Struct("device_p9", [
>      ("path",             string),
>      ("security_model",   string),
>      ("devid",            libxl_devid),
> +    ("type",             libxl_p9_type),
> +    ("max_space",        integer),
> +    ("max_files",        integer),
> +    ("max_open_files",   integer),
> +    ("auto_delete",      bool),

These additional fields and options probably a
LIBXL_HAVE_XEN_9PFS macro in libxl.h.


Thanks,

-- 
Anthony PERARD