From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wei Liu <wei.liu2@citrix.com>
Subject: Re: [PATCH v4 3/5] libxl: add support for vscsi
Date: Tue, 5 May 2015 14:55:37 +0100
Message-ID: <20150505135537.GE1455@zion.uk.xensource.com>
References: <1429259460-30491-1-git-send-email-olaf@aepfle.de>
	<1429259460-30491-4-git-send-email-olaf@aepfle.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <1429259460-30491-4-git-send-email-olaf@aepfle.de>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Olaf Hering <olaf@aepfle.de>
Cc: Wei Liu <wei.liu2@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Ian Campbell <ian.campbell@citrix.com>, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

On Fri, Apr 17, 2015 at 08:30:58AM +0000, 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@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  docs/man/xl.cfg.pod.5                |  55 +++
>  docs/man/xl.pod.1                    |  18 +
>  tools/libxl/Makefile                 |   2 +
>  tools/libxl/libxl.c                  | 441 ++++++++++++++++++++
>  tools/libxl/libxl.h                  |  27 ++
>  tools/libxl/libxl_create.c           |   1 +
>  tools/libxl/libxl_device.c           |   2 +
>  tools/libxl/libxl_internal.h         |  16 +
>  tools/libxl/libxl_types.idl          |  56 +++
>  tools/libxl/libxl_types_internal.idl |   1 +
>  tools/libxl/libxl_vscsi.c            | 271 +++++++++++++
>  tools/libxl/libxlu_vscsi.c           | 750 +++++++++++++++++++++++++++++++++++
>  tools/libxl/libxlutil.h              |  21 +
>  tools/libxl/xl.h                     |   3 +
>  tools/libxl/xl_cmdimpl.c             | 184 ++++++++-
>  tools/libxl/xl_cmdtable.c            |  15 +
>  16 files changed, 1862 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index f936dfc..d395e56 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -510,6 +510,61 @@ value is optional if this is a guest domain.
>  
>  =back
>  
> +=item B<vscsi=[ "VSCSI_SPEC_STRING", "VSCSI_SPEC_STRING", ...]>
> +
> +Specifies the PVSCSI devices to be provided to the guest. PVSCSI passes
> +dom0 SCSI devices as-is to the guest.
> +
> +Each VSCSI_SPEC_STRING consists of "pdev,vdev[,options]".
> +'pdev' describes the physical device, preferable in a persistant format.

"persistent", and please explain when "persistent format" is.

> +'vdev' is the domU device in vHOST:CHANNEL:TARGET:LUN notation, all integers.
> +'option' lists additional flags which a backend may recognize.
> +
> +The supported values for "pdev" and "option" depends on the used backend driver:
> +
> +=over 4
> +
> +=item B<Linux pvops>
> +
> +=over 4
> +
> +=item C<pdev>
> +
> +The backend driver in the pvops kernel is part of the Linux-IO Target framework
> +(LIO). As such the SCSI devices have to be configured first with the tools
> +provided by this framework, such as a xen-scsiback aware targetcli. The "pdev"

What is "targetcli"?

> +in domU.cfg has to refer to a config item in that framework instead of the raw
> +device. Ususally this is a WWN in the form of "na.WWN:LUN".
> +
> +=item C<option>
> +
> +No options recognized.
> +
> +=back
> +
> +=item B<Linux xenlinux>
> +
> +=over 4
> +
> +=item C<pdev>
> +
> +The dom0 device in either /dev/scsidev or pHOST:CHANNEL:TARGET:LUN notation.
> +

What is vHOST? What is pHOST?

> +Its recommended to use persistant names "/dev/disk/by-*/*" to refer to a "pdev".

"It's"  "persistent"

> +The toolstack will translate this internally to "h:c:t:l" notation, which is how
> +the backend driver will access the device. Using the "h:c:t:l" notation for
> +"pdev" in domU.cfg is discouraged because this value will change across reboots,
> +depending on the detection order in the OS.
> +
> +=item C<option>
> +
> +Currently only the option value "feature-host" is recognized. SCSI command
> +emulation in backend driver is bypassed when "feature-host" is specified.
> +
> +=back
> +
> +=back
> +
>  =item B<vfb=[ "VFB_SPEC_STRING", "VFB_SPEC_STRING", ...]>
>  
>  Specifies the paravirtual framebuffer devices which should be supplied
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 16783c8..19bdbfa 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1328,6 +1328,24 @@ List virtual trusted platform modules for a domain.
>  
>  =back
>  
> +=head2 PVSCSI DEVICES
> +
> +=over 4
> +
> +=item B<scsi-attach> I<domain-id> I<pdev> I<vdev>,I<[feature-host]>
> +
> +Creates a new vscsi device in the domain specified by I<domain-id>.
> +
> +=item B<scsi-detach> I<domain-id> I<vdev>
> +
> +Removes the vscsi device from domain specified by I<domain-id>.
> +
> +=item B<scsi-list> I<domain-id> I<[domain-id] ...>
> +
> +List vscsi devices for the domain specified by I<domain-id>.
> +
> +=back
> +
>  =head1 PCI PASS-THROUGH
>  
>  =over 4
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 1b16598..79b3867 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -91,6 +91,7 @@ endif
>  LIBXL_LIBS += -lyajl
>  
>  LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
> +			libxl_vscsi.o \
>  			libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
>  			libxl_internal.o libxl_utils.o libxl_uuid.o \
>  			libxl_json.o libxl_aoutils.o libxl_numa.o libxl_vnuma.o \
> @@ -122,6 +123,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
>  AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
>  AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
>  LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
> +	libxlu_vscsi.o \
>  	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
>  $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
>  
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 511eef1..abe7d75 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2014,6 +2014,437 @@ static int libxl__resolve_domid(libxl__gc *gc, const char *name,
>  }
>  
>  /******************************************************************************/
> +
> +static void libxl__device_vscsi_dev_backend_rm(libxl__gc *gc,
> +                                              libxl_vscsi_dev *v,
> +                                              xs_transaction_t t,
> +                                              char *be_path,

const char *be_path

> +                                              int dev_wait)
> +{
> +    char *path, *val;
> +    libxl_ctx *ctx = libxl__gc_owner(gc);

You can just use CTX macro.

> +
> +    path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
> +    val = libxl__xs_read(gc, t, path);
> +    LOG(DEBUG, "%s is %s", path, val);
> +    if (val && strcmp(val, GCSPRINTF("%d", dev_wait)) == 0) {
> +        path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
> +        xs_rm(ctx->xsh, t, path);
> +        path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-devname", be_path, v->vscsi_dev_id);
> +        xs_rm(ctx->xsh, t, path);
> +        path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev", be_path, v->vscsi_dev_id);
> +        xs_rm(ctx->xsh, t, path);
> +        path = GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev", be_path, v->vscsi_dev_id);
> +        xs_rm(ctx->xsh, t, path);
> +        path = GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id);
> +        xs_rm(ctx->xsh, t, path);

Some lines are too long. Please wrap them properly.

But can you not simplify this by removing be_path all together?

Also this function should return error number -- you probably don't want
caller to commence should things go wrong here.

> +    } else {
> +        LOG(ERROR, "%s has %s, expected %d", path, val, dev_wait);
> +    }
> +}
> +
> +static int libxl__device_vscsi_dev_backend_set(libxl__gc *gc,
> +                                               libxl_vscsi_dev *v,
> +                                               flexarray_t *back)
> +{
> +    int rc;
> +    libxl_vscsi_hctl *hctl;
> +
> +    switch (v->pdev.type) {
> +        case LIBXL_VSCSI_PDEV_TYPE_WWN:
> +            flexarray_append_pair(back,
> +                                  GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
> +                                  v->pdev.u.wwn.m);
> +            break;
> +        case LIBXL_VSCSI_PDEV_TYPE_HCTL:
> +            hctl = &v->pdev.u.hctl.m;
> +            flexarray_append_pair(back,
> +                                  GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
> +                                  GCSPRINTF("%u:%u:%u:%u", hctl->hst, hctl->chn, hctl->tgt, hctl->lun));
> +            break;
> +        case LIBXL_VSCSI_PDEV_TYPE_INVALID:
> +        default:
> +            rc = ERROR_FAIL;
> +            goto out;
> +    }
> +    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-devname", v->vscsi_dev_id),
> +                          v->pdev.p_devname);
> +    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/v-dev", v->vscsi_dev_id),
> +                          GCSPRINTF("%u:%u:%u:%u", v->vdev.hst, v->vdev.chn, v->vdev.tgt, v->vdev.lun));
> +    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id),
> +                          GCSPRINTF("%d", XenbusStateInitialising));

Please fix the lines that are too long.

> +    rc = 0;
> +out:
> +    return rc;
> +}
> +
> +static int libxl__device_vscsi_new_backend(libxl__egc *egc,
> +                                           libxl__ao_device *aodev,
> +                                           libxl_device_vscsi *vscsi,
> +                                           libxl_domain_config *d_config)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    int rc, i;
> +    flexarray_t *back;
> +    flexarray_t *front;
> +    libxl_vscsi_dev *v;
> +    xs_transaction_t t = XBT_NULL;
> +
> +    /* Prealloc key+value: 4 toplevel + 4 per device */
> +    i = 2 * (4 + (4 * vscsi->num_vscsi_devs));
> +    back = flexarray_make(gc, i, 1);
> +    front = flexarray_make(gc, 2 * 2, 1);
> +
> +    flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", aodev->dev->domid));
> +    flexarray_append_pair(back, "online", "1");
> +    flexarray_append_pair(back, "state", GCSPRINTF("%d", XenbusStateInitialising));
> +    flexarray_append_pair(back, "feature-host",
> +                          libxl_defbool_val(vscsi->feature_host) ?
> +                          "1" : "0");
> +
> +    flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
> +    flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising));
> +

Lines too long.

> +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> +        v = vscsi->vscsi_devs + i;
> +        if (v->remove)
> +            continue;
> +        rc = libxl__device_vscsi_dev_backend_set(gc, v, back);
> +        if (rc) goto out;
> +    }
> +
> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) goto out;
> +
> +        rc = libxl__device_exists(gc, t, aodev->dev);
> +        if (rc < 0) goto out;
> +        if (rc == 1) {              /* already exists in xenstore */
> +            LOG(ERROR, "device already exists in xenstore");
> +            rc = ERROR_DEVICE_EXISTS;
> +            goto out;
> +        }
> +
> +        if (aodev->update_json) {
> +            rc = libxl__set_domain_configuration(gc, aodev->dev->domid, d_config);
> +            if (rc) goto out;
> +        }
> +
> +        libxl__device_generic_add(gc, t, aodev->dev,
> +                                  libxl__xs_kvs_of_flexarray(gc, back,
> +                                                             back->count),
> +                                  libxl__xs_kvs_of_flexarray(gc, front,
> +                                                             front->count),
> +                                  NULL);
> +
> +        rc = libxl__xs_transaction_commit(gc, &t);
> +        if (!rc) break;
> +        if (rc < 0) goto out;
> +    }
> +
> +    libxl__wait_device_connection(egc, aodev);
> +    rc = 0;
> +
> +out:
> +    libxl__xs_transaction_abort(gc, &t);
> +    return rc;
> +}
> +
> +static int libxl__device_vscsi_reconfigure(libxl__egc *egc,
> +                                           libxl__ao_device *aodev,
> +                                           libxl_device_vscsi *vscsi,
> +                                           libxl_domain_config *d_config,
> +                                           char *be_path,

const char *

> +                                           int *dev_wait)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    int rc, i, be_state, be_wait;
> +    char *dev_path, *state_path, *state_val;
> +    flexarray_t *back;
> +    libxl_vscsi_dev *v;
> +    xs_transaction_t t = XBT_NULL;
> +    bool do_reconfigure = false;
> +
> +    /* Prealloc key+value: 1 toplevel + 4 per device */
> +    i = 2 * (1 + (4 * vscsi->num_vscsi_devs));
> +    back = flexarray_make(gc, i, 1);
> +
> +    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;
> +                if (dev_wait)
> +                    *dev_wait = XenbusStateInitialising;
> +                do_reconfigure = false;
> +                break;
> +            case XenbusStateConnected:
> +                /* The backend can handle reconfigure */
> +                be_wait = XenbusStateConnected;
> +                if (dev_wait)
> +                    *dev_wait = XenbusStateClosed;
> +                flexarray_append_pair(back, "state", GCSPRINTF("%d", XenbusStateReconfiguring));
> +                do_reconfigure = true;
> +                break;
> +        }
> +
> +        /* Append new device or trigger removal */
> +        for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> +            v = vscsi->vscsi_devs + i;
> +            unsigned int nb = 0;

Move this line at the beginning of this block. I think you will hit a
gcc warning here, won't you?

> +            dev_path = GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id);
> +            /* Preserve existing device */
> +            if (libxl__xs_directory(gc, XBT_NULL, dev_path, &nb) && nb) {
> +                /* Trigger device removal by forwarding state to XenbusStateClosing */
> +                if (do_reconfigure && v->remove)
> +                    flexarray_append_pair(back,
> +                                          GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id),
> +                                          GCSPRINTF("%d", XenbusStateClosing));
> +                continue;
> +            }
> +            rc = libxl__device_vscsi_dev_backend_set(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) {
> +        /* Poll for backend change */
> +        rc = libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", be_wait));
> +        if (rc) goto out;
> +    }
> +    rc = 0;
> +
> +out:
> +    LOG(ERROR, "%u: rc %d", __LINE__, rc);

Remove this line.

> +    libxl__xs_transaction_abort(gc, &t);
> +    return rc;
> +}
> +
> +static int libxl__device_from_vscsi(libxl__gc *gc, uint32_t domid,
> +                                    libxl_device_vscsi *vscsi,
> +                                    libxl__device *device)
> +{
> +    device->backend_domid = vscsi->backend_domid;
> +    device->devid         = vscsi->devid;
> +    device->domid         = domid;
> +    device->backend_kind  = LIBXL__DEVICE_KIND_VSCSI;
> +    device->kind          = LIBXL__DEVICE_KIND_VSCSI;
> +
> +    return 0;
> +}
> +
> +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> +                             libxl_device_vscsi *vscsi,
> +                             libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    libxl__device *device;
> +    char *be_path;
> +    unsigned int be_dirs = 0;
> +    int rc;
> +    libxl_domain_config d_config;
> +    libxl_device_vscsi vscsi_saved;
> +    libxl__domain_userdata_lock *lock = NULL;
> +
> +    libxl_domain_config_init(&d_config);
> +
> +    /* In case of vscsi the copy remains identical to the provided input */

This comment is confusing.

> +    libxl_device_vscsi_init(&vscsi_saved);
> +    libxl_device_vscsi_copy(CTX, &vscsi_saved, vscsi);
> +
> +    if (vscsi->devid == -1) {
> +        if ((vscsi->devid = libxl__device_nextid(gc, domid, "vscsi")) < 0) {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +    }
> +
> +    /* Adjust copy */
> +    libxl__update_config_vscsi(gc, &vscsi_saved, vscsi);
> +
> +    GCNEW(device);
> +    rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
> +    if (rc) goto out;
> +
> +    if (aodev->update_json) {
> +        lock = libxl__lock_domain_userdata(gc, domid);
> +        if (!lock) {
> +            rc = ERROR_LOCK_FAIL;
> +            goto out;
> +        }
> +
> +        rc = libxl__get_domain_configuration(gc, domid, &d_config);
> +        if (rc) goto out;
> +
> +        /* Replace or append the copy to the domain config */
> +        DEVICE_ADD(vscsi, vscsis, domid, &vscsi_saved, COMPARE_VSCSI, &d_config);
> +    }
> +
> +    aodev->dev = device;
> +
> +    be_path = libxl__device_backend_path(gc, aodev->dev);
> +    if (libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs)) {

This looks bogus. Shouldn't you use a real transaction here and in the
following two functions?

> +        rc = libxl__device_vscsi_reconfigure(egc, aodev, vscsi, &d_config, be_path, NULL);
> +        if (rc)
> +            goto out;
> +        /* Notify that this is done */
> +        aodev->callback(egc, aodev);

And this is for? You can just do this in out path, right?

> +    } else {
> +        rc = libxl__device_vscsi_new_backend(egc, aodev, vscsi, &d_config);
> +        if (rc)
> +            goto out;
> +    }
> +
> +    rc = 0;
> +out:
> +    if (lock) libxl__unlock_domain_userdata(lock);
> +    libxl_device_vscsi_dispose(&vscsi_saved);
> +    libxl_domain_config_dispose(&d_config);
> +    aodev->rc = rc;
> +    if (rc) aodev->callback(egc, aodev);
> +    return;
> +}
> +
> +static void libxl__device_vscsi_dev_rm(libxl__egc *egc,
> +                                       libxl_device_vscsi *vscsi,
> +                                       libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    char *be_path;
> +    int rc, i, dev_wait;
> +    libxl_domain_config d_config;
> +    libxl__domain_userdata_lock *lock = NULL;
> +    libxl_vscsi_dev *v;
> +    xs_transaction_t t = XBT_NULL;
> +
> +    libxl_domain_config_init(&d_config);
> +
> +    if (vscsi->devid == -1) {
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    /* No other code will traverse device list, update json with removal info */
> +    if (aodev->update_json) {

When is update_json set to true?

Note that in DEFINE_DEVICE_ADD update_json is set to true. But in you
patch there doesn't seem to be code that does this.

> +        lock = libxl__lock_domain_userdata(gc, aodev->dev->domid);
> +        if (!lock) {
> +            rc = ERROR_LOCK_FAIL;
> +            goto out;
> +        }
> +
> +        rc = libxl__get_domain_configuration(gc, aodev->dev->domid, &d_config);
> +        if (rc) goto out;
> +
> +        /* Replace the item in the domain config */
> +        DEVICE_ADD(vscsi, vscsis, aodev->dev->domid, vscsi, COMPARE_VSCSI, &d_config);
> +    }
> +

Actually, what fields inside vscsi structure are updated in this case?

> +    be_path = libxl__device_backend_path(gc, aodev->dev);
> +    rc = libxl__device_vscsi_reconfigure(egc, aodev, vscsi, &d_config, be_path, &dev_wait);
> +    if (rc) goto out;
> +
> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) goto out;
> +
> +        for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> +            v = vscsi->vscsi_devs + i;
> +            if (v->remove)
> +                libxl__device_vscsi_dev_backend_rm(gc, v, t, be_path, dev_wait);
> +        }
> +
> +        rc = libxl__xs_transaction_commit(gc, &t);
> +        if (!rc) break;
> +        if (rc < 0) goto out;
> +    }
> +
> +    rc = 0;
> +
> +out:
> +    if (lock) libxl__unlock_domain_userdata(lock);
> +    libxl_domain_config_dispose(&d_config);
> +    aodev->rc = rc;
> +    /* Notify that this is done */
> +    aodev->callback(egc, aodev);
> +}
> +
> +/* Extended variant of DEFINE_DEVICE_REMOVE to handle reconfigure */

I have a very dumb question, what is "reconfigure"? Is this property of
vscsi device? What does this suppose to do?


> +                              libxl_device_vscsi *vscsi,
> +                              const libxl_asyncop_how *ao_how)
> +{
> +    AO_CREATE(ctx, domid, ao_how);
> +    libxl__device *device;
> +    libxl__ao_device *aodev;
> +    int rc, i;
> +    unsigned int remaining = vscsi->num_vscsi_devs;
> +
> +    GCNEW(device);
> +    rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
> +    if (rc != 0) goto out;
> +
> +    GCNEW(aodev);
> +    libxl__prepare_ao_device(ao, aodev);
> +    aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
> +    aodev->dev = device;
> +    aodev->callback = device_addrm_aocomplete;
> +    aodev->force = 0;
> +
> +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> +        if (vscsi->vscsi_devs[i].remove)
> +            remaining--;
> +    }
> +
> +    LOG(DEBUG, "%u: v_hst %u, %u of %u remaining", domid, vscsi->v_hst, remaining, vscsi->num_vscsi_devs);
> +    if (remaining)
> +        libxl__device_vscsi_dev_rm(egc, vscsi, aodev);
> +    else
> +        libxl__initiate_device_remove(egc, aodev);
> +
> +out:
> +    if (rc) return AO_ABORT(rc);
> +    return AO_INPROGRESS;
> +}
> +

The rest of this patch is mostly parser, declarations and xl stuff. I've
omitted the reset for now...

Wei.