Re: [Xen-devel] [PATCH v12 1/2] libxl: add support for vscsi

[Juergen Gross <jgross@xxxxxxxx>]

On 13/04/16 10:56, Olaf Hering wrote:
> Port pvscsi support from xend to libxl:
> 
>  vscsi=['pdev,vdev{,options}']
>  xl scsi-attach
>  xl scsi-detach
>  xl scsi-list
> 
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  docs/man/xl.cfg.pod.5                |   56 ++
>  docs/man/xl.pod.1                    |   18 +
>  tools/libxl/Makefile                 |    2 +
>  tools/libxl/libxl.c                  |    9 +
>  tools/libxl/libxl.h                  |   42 ++
>  tools/libxl/libxl_create.c           |   41 +-
>  tools/libxl/libxl_device.c           |    2 +
>  tools/libxl/libxl_internal.h         |    8 +
>  tools/libxl/libxl_types.idl          |   53 ++
>  tools/libxl/libxl_types_internal.idl |    1 +
>  tools/libxl/libxl_vscsi.c            | 1169 
> ++++++++++++++++++++++++++++++++++
>  tools/libxl/libxlu_vscsi.c           |  667 +++++++++++++++++++
>  tools/libxl/libxlutil.h              |   19 +
>  tools/libxl/xl.h                     |    3 +
>  tools/libxl/xl_cmdimpl.c             |  225 ++++++-
>  tools/libxl/xl_cmdtable.c            |   15 +
>  16 files changed, 2326 insertions(+), 4 deletions(-)
> 

...

> diff --git a/tools/libxl/libxl_vscsi.c b/tools/libxl/libxl_vscsi.c
> new file mode 100644
> index 0000000..fff8c7a
> --- /dev/null
> +++ b/tools/libxl/libxl_vscsi.c

...

> +static void vscsictrl_do_reconfigure_add_cb(libxl__egc *egc,
> +                                            libxl__ev_devstate *ds,
> +                                            int rc)
> +{
> +    libxl__ao_device *aodev = CONTAINER_OF(ds, *aodev, backend_ds);
> +    STATE_AO_GC(aodev->ao);
> +    aodev->rc = rc;
> +    aodev->callback(egc, aodev);
> +}
> +
> +static void vscsictrl_do_reconfigure_add(libxl__egc *egc,
> +                                         libxl__ao_device *aodev,
> +                                         libxl_device_vscsictrl *vscsictrl,
> +                                         libxl_domain_config *d_config)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    int rc, i, be_state, be_wait;
> +    const char *be_path;
> +    char *dev_path, *state_path, *state_val;
> +    flexarray_t *back;
> +    libxl_device_vscsidev *v;
> +    xs_transaction_t t = XBT_NULL;
> +    bool do_reconfigure = false;
> +
> +    /* Prealloc key+value: 1 toplevel + 4 per device */
> +    i = 2 * (1 + (4 * vscsictrl->num_vscsidevs));
> +    back = flexarray_make(gc, i, 1);

Shouldn't be this in the loop below? Otherwise the array might be
expanded with the same entries several times in case of a transaction
collision.

I've found other places in libxl_vscsi.c with the same problem.

> +
> +    be_path = libxl__device_backend_path(gc, aodev->dev);
> +    state_path = GCSPRINTF("%s/state", be_path);
> +
> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) goto out;
> +
> +        state_val = libxl__xs_read(gc, t, state_path);
> +        LOG(DEBUG, "%s is %s", state_path, state_val);
> +        if (!state_val) {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +
> +        be_state = atoi(state_val);
> +        switch (be_state) {
> +        case XenbusStateUnknown:
> +        case XenbusStateInitialising:
> +        case XenbusStateClosing:
> +        case XenbusStateClosed:
> +        default:
> +            /* The backend is in a bad state */
> +            rc = ERROR_FAIL;
> +            goto out;
> +        case XenbusStateInitialised:
> +        case XenbusStateReconfiguring:
> +        case XenbusStateReconfigured:
> +            /* Backend is still busy, caller has to retry */
> +            rc = ERROR_NOT_READY;
> +            goto out;
> +        case XenbusStateInitWait:
> +            /* The frontend did not connect yet */
> +            be_wait = XenbusStateInitWait;
> +            do_reconfigure = false;
> +            break;
> +        case XenbusStateConnected:
> +            /* The backend can handle reconfigure */
> +            be_wait = XenbusStateConnected;
> +            flexarray_append_pair(back, "state", GCSPRINTF("%d", 
> XenbusStateReconfiguring));
> +            do_reconfigure = true;
> +            break;
> +        }
> +
> +        /* Append new vscsidev or skip existing  */
> +        for (i = 0; i < vscsictrl->num_vscsidevs; i++) {
> +            unsigned int nb = 0;
> +            v = vscsictrl->vscsidevs + i;
> +            dev_path = GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, 
> v->vscsidev_id);
> +            if (libxl__xs_directory(gc, XBT_NULL, dev_path, &nb)) {
> +                /* FIXME Sanity check */
> +                LOG(DEBUG, "%s exists already with %u entries", dev_path, 
> nb);
> +                continue;
> +            }
> +            rc = vscsidev_backend_add(gc, v, back);
> +            if (rc) goto out;
> +        }
> +
> +        if (aodev->update_json) {
> +            rc = libxl__set_domain_configuration(gc, aodev->dev->domid, 
> d_config);
> +            if (rc) goto out;
> +        }
> +
> +        libxl__xs_writev(gc, t, be_path,
> +                         libxl__xs_kvs_of_flexarray(gc, back, back->count));
> +
> +        rc = libxl__xs_transaction_commit(gc, &t);
> +        if (!rc) break;
> +        if (rc < 0) goto out;
> +    }
> +
> +    if (do_reconfigure) {
> +        rc = libxl__ev_devstate_wait(ao, &aodev->backend_ds,
> +                                     vscsictrl_do_reconfigure_add_cb,
> +                                     state_path, be_wait,
> +                                     LIBXL_INIT_TIMEOUT * 1000);
> +        if (rc) goto out;
> +    }
> +    return;
> +
> +out:
> +    libxl__xs_transaction_abort(gc, &t);
> +    aodev->rc = rc;
> +    aodev->callback(egc, aodev);
> +}

Juergen


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