|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 3/5] libxl: add support for vscsi
I notice you posted several enquiries in your previous patch, but I'm
not sure if any of them still requires answers because you seemed to
have posted answers to your own enquiries. I skip all of them and start
from this version.
There seems to be a lot of places in this patch where the lines are
longer than 80 characters. I'm not going to point them out one by one.
Can you fix as many of those as you can where sensible?
Because VSCSI is similar to PVUSB and PVUSB has mostly been reviewed, I
basically did a side by side comparison between these two. My goal is to
keep them as close as possible.
I'm also aware VSCSI is not complete the same as PVUSB. If my comments
are made based on wrong assumptions, please let me know.
On Thu, Feb 11, 2016 at 03:43:29PM +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@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 | 55 +++
> docs/man/xl.pod.1 | 18 +
> tools/libxl/Makefile | 2 +
> tools/libxl/libxl.c | 420 +++++++++++++++++++++
> tools/libxl/libxl.h | 35 ++
> tools/libxl/libxl_create.c | 1 +
> tools/libxl/libxl_device.c | 2 +
> tools/libxl/libxl_internal.h | 19 +
> tools/libxl/libxl_types.idl | 53 +++
> tools/libxl/libxl_types_internal.idl | 1 +
> tools/libxl/libxl_vscsi.c | 615 +++++++++++++++++++++++++++++++
> tools/libxl/libxlu_vscsi.c | 697
> +++++++++++++++++++++++++++++++++++
> tools/libxl/libxlutil.h | 18 +
> tools/libxl/xl.h | 3 +
> tools/libxl/xl_cmdimpl.c | 208 ++++++++++-
> tools/libxl/xl_cmdtable.c | 15 +
> 16 files changed, 2161 insertions(+), 1 deletion(-)
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 8899f75..3b5b1c5 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -517,6 +517,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
> +SCSI devices from the backend domain to the guest.
> +
> +Each VSCSI_SPEC_STRING consists of "pdev,vdev[,options]".
> +'pdev' describes the physical device, preferable in a persistent format such
> as /dev/disk/by-*/*.
Small nit: please wrap this line a bit.
> +'vdev' is the domU device in vHOST:CHANNEL:TARGET:LUN notation, all integers.
> +'options' lists additional flags which a backend may recognize.
> +
> +The supported values for "pdev" and "options" depends on the backend driver
> used:
> +
> +=over 4
> +
> +=item B<Linux>
> +
> +=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"
> +in domU.cfg has to refer to a config item in that framework instead of the
> raw
> +device. Usually this is a WWN in the form of "na.WWN:LUN".
> +
naa
> +=item C<options>
> +
> +No options recognized.
> +
> +=back
> +
> +=item B<Linux based on classic Xen kernel>
> +
> +=over 4
> +
> +=item C<pdev>
> +
[...]
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2bde0f5..7583b4b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2059,6 +2059,417 @@ static int libxl__resolve_domid(libxl__gc *gc, const
> char *name,
> }
>
>
> /******************************************************************************/
> +
> +static int libxl__device_vscsidev_backend_set_add(libxl__gc *gc,
> + libxl_device_vscsidev *v,
> + flexarray_t *back)
Indentation.
> +{
> + int rc;
> + char *dir;
> + unsigned int hst, chn, tgt;
> + unsigned long long lun;
> +
> +
> + dir = GCSPRINTF("vscsi-devs/dev-%u", v->vscsidev_id);
> + switch (v->pdev.type) {
> + case LIBXL_VSCSI_PDEV_TYPE_WWN:
> + flexarray_append_pair(back,
> + GCSPRINTF("%s/p-dev", dir),
> + v->pdev.u.wwn.m);
> + break;
> + case LIBXL_VSCSI_PDEV_TYPE_HCTL:
> + hst = v->pdev.u.hctl.m.hst;
> + chn = v->pdev.u.hctl.m.chn;
> + tgt = v->pdev.u.hctl.m.tgt;
> + lun = v->pdev.u.hctl.m.lun;
> + flexarray_append_pair(back,
> + GCSPRINTF("%s/p-dev", dir),
> + GCSPRINTF("%u:%u:%u:%llu", hst, chn, tgt,
> lun));
> + break;
> + case LIBXL_VSCSI_PDEV_TYPE_INVALID:
Please add /* fallthrough */ here, otherwise Coverity scan might
complain. :-/
> + default:
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + flexarray_append_pair(back,
> + GCSPRINTF("%s/p-devname", dir),
> + v->pdev.p_devname);
> + hst = v->vdev.hst;
> + chn = v->vdev.chn;
> + tgt = v->vdev.tgt;
> + lun = v->vdev.lun;
> + flexarray_append_pair(back,
> + GCSPRINTF("%s/v-dev", dir),
> + GCSPRINTF("%u:%u:%u:%llu", hst, chn, tgt, lun));
> + flexarray_append_pair(back,
> + GCSPRINTF("%s/state", dir),
> + GCSPRINTF("%d", XenbusStateInitialising));
> + rc = 0;
> +out:
> + return rc;
> +}
> +
> +static int libxl__device_vscsi_new_backend(libxl__egc *egc,
> + libxl__ao_device *aodev,
> + libxl_device_vscsictrl *vscsi,
> + libxl_domain_config *d_config)
> +{
> + STATE_AO_GC(aodev->ao);
> + int rc, i;
> + flexarray_t *back;
> + flexarray_t *front;
> + libxl_device_vscsidev *v;
> + xs_transaction_t t = XBT_NULL;
> +
> + /* Prealloc key+value: 4 toplevel + 4 per device */
> + i = 2 * (4 + (4 * vscsi->num_vscsidevs));
> + 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->scsi_raw_cmds) ?
> + "1" : "0");
> +
> + flexarray_append_pair(front, "backend-id", GCSPRINTF("%d",
> vscsi->backend_domid));
> + flexarray_append_pair(front, "state", GCSPRINTF("%d",
> XenbusStateInitialising));
> +
Wrap long lines please.
> + for (i = 0; i < vscsi->num_vscsidevs; i++) {
> + v = vscsi->vscsidevs + i;
> + rc = libxl__device_vscsidev_backend_set_add(gc, v, back);
> + if (rc) return rc;
goto out please.
> + }
> +
> + 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;
> +}
> +
[...]
> +}
> +void libxl__device_vscsictrl_add(libxl__egc *egc, uint32_t domid,
> + libxl_device_vscsictrl *vscsi,
> + libxl__ao_device *aodev)
> +{
> + STATE_AO_GC(aodev->ao);
> + libxl__device *device;
> + const char *be_path;
> + unsigned int be_dirs = 0;
> + int rc;
> + libxl_domain_config d_config;
> + libxl_device_vscsictrl vscsi_saved;
> + libxl__domain_userdata_lock *lock = NULL;
> +
> + libxl_domain_config_init(&d_config);
> +
> + libxl_device_vscsictrl_init(&vscsi_saved);
> + libxl_device_vscsictrl_copy(CTX, &vscsi_saved, vscsi);
> +
> + if (vscsi->devid == -1) {
> + LOGE(ERROR, "new vscsictrl for domid %u has uninitialized devid",
> domid);
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + GCNEW(device);
> + rc = libxl__device_from_vscsictrl(gc, domid, vscsi, device);
> + if (rc) goto out;
> +
> + aodev->dev = device;
> + be_path = libxl__device_backend_path(gc, aodev->dev);
> +
> + rc = libxl__vscsi_assign_vscsidev_ids(gc, domid, &vscsi_saved);
> + 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(vscsictrl, vscsictrls, domid, &vscsi_saved,
> COMPARE_VSCSI, &d_config);
> + }
> +
> + if (libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs)) {
> + rc = libxl__device_vscsi_reconfigure_add(egc, aodev, &vscsi_saved,
> &d_config, be_path);
> + if (rc)
> + goto out;
> + /* Notify that this is done */
> + aodev->callback(egc, aodev);
Wouldn't it be better if you call aodev->callback unconditionally at the
end of this function? BTW you seem to have forgotten to set aodev->rc.
> + } else {
> + rc = libxl__device_vscsi_new_backend(egc, aodev, &vscsi_saved,
> &d_config);
> + if (rc)
> + goto out;
> + }
> +
> + rc = 0;
> +out:
> + if (lock) libxl__unlock_domain_userdata(lock);
> + libxl_device_vscsictrl_dispose(&vscsi_saved);
> + libxl_domain_config_dispose(&d_config);
> + aodev->rc = rc;
> + if (rc) aodev->callback(egc, aodev);
> + return;
> +}
> +
> +
[...]
> +/* Virtual SCSI */
> +int libxl_device_vscsictrl_add(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_vscsictrl *vscsi,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vscsictrl_remove(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_vscsictrl *vscsi,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vscsictrl_destroy(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_vscsictrl *vscsi,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +libxl_device_vscsictrl *libxl_device_vscsictrl_list(libxl_ctx *ctx, uint32_t
> domid, int *num);
> +int libxl_device_vscsictrl_getinfo(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_vscsictrl *vscsictrl,
> + libxl_device_vscsidev *vscsidev,
> + libxl_vscsiinfo *vscsiinfo);
> +/* Remove vscsidev connected to vscsictrl */
> +int libxl_device_vscsidev_remove(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_vscsidev *dev,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
Where is libxl_device_vscsidev_add?
> +void libxl_device_vscsictrl_append_vscsidev(libxl_ctx *ctx,
> + libxl_device_vscsictrl *ctrl,
> + libxl_device_vscsidev *dev);
> +
> /* Virtual TPMs */
> int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm
> *vtpm,
> const libxl_asyncop_how *ao_how)
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index e491d83..64f3c70 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1166,6 +1166,7 @@ static void domcreate_rebuild_done(libxl__egc *egc,
> libxl__multidev_begin(ao, &dcs->multidev);
> dcs->multidev.callback = domcreate_launch_dm;
> libxl__add_disks(egc, ao, domid, d_config, &dcs->multidev);
> + libxl__add_vscsictrls(egc, ao, domid, d_config, &dcs->multidev);
This is because you need vscsi disk early?
> libxl__multidev_prepared(egc, &dcs->multidev, 0);
>
> return;
[...]
> libxl__console_backend = Enumeration("console_backend", [
> diff --git a/tools/libxl/libxl_vscsi.c b/tools/libxl/libxl_vscsi.c
[...]
> +static bool libxl__vscsi_fill_dev(libxl__gc *gc,
> + xs_transaction_t t,
> + const char *devs_path,
> + const char *dev_dir,
> + libxl_device_vscsidev *dev)
> +{
> + char *path, *c, *p, *v, *s;
> + unsigned int devid;
> + int r;
> +
> + r = sscanf(dev_dir, "dev-%u", &devid);
> + if (r != 1) {
> + LOG(ERROR, "expected dev-N, got '%s'", dev_dir);
> + return false;
> + }
> + dev->vscsidev_id = devid;
> +
> + path = GCSPRINTF("%s/%s", devs_path, dev_dir);
> + c = libxl__xs_read(gc, t, GCSPRINTF("%s/p-devname", path));
> + p = libxl__xs_read(gc, t, GCSPRINTF("%s/p-dev", path));
> + v = libxl__xs_read(gc, t, GCSPRINTF("%s/v-dev", path));
> + s = libxl__xs_read(gc, t, GCSPRINTF("%s/state", path));
> + LOG(DEBUG, "%s/state is %s", path, s);
> + if (!(c && p && v && s)) {
> + LOG(ERROR, "p-devname '%s' p-dev '%s' v-dev '%s'", c, p, v);
> + return false;
> + }
> +
> + if (!vscsi_parse_pdev(gc, dev, c, p, v)) {
> + LOG(ERROR, "failed to parse %s: %s %s %s %s", path, c, p, v, s);
> + return false;
> + }
> +
> + switch (atoi(s)) {
> + case XenbusStateUnknown:
> + case XenbusStateInitialising:
> + case XenbusStateInitWait:
> + case XenbusStateInitialised:
> + case XenbusStateConnected:
> + case XenbusStateReconfiguring:
> + case XenbusStateReconfigured:
> + break;
> + case XenbusStateClosing:
> + case XenbusStateClosed:
> + LOG(DEBUG, "unexpected state in %s: %s", path, s);
> + break;
> + }
> +
This doesn't seem useful in the context of this function.
> + return true;
> +}
> +
> +static bool libxl__vscsi_fill_ctrl(libxl__gc *gc,
> + xs_transaction_t t,
> + const char *fe_path,
> + const char *dir,
> + libxl_device_vscsictrl *ctrl)
> +{
> + libxl_device_vscsidev dev;
> + char *tmp, *be_path, *devs_path;
> + char **dev_dirs;
> + unsigned int ndev_dirs, dev_dir;
> + bool parsed_ok;
> +
> + ctrl->devid = atoi(dir);
> +
> + be_path = libxl__xs_read(gc, t, GCSPRINTF("%s/%s/backend", fe_path,
> dir));
> + /* FIXME what if xenstore is broken? */
I'm not sure what kind of "broken" you referred to, but it is most
likely we can't recover from such state.
It's reasonable to assume xenstore is robust.
> + if (!be_path) {
> + libxl_defbool_set(&ctrl->scsi_raw_cmds, false);
> + return false;
> + }
> +
> + tmp = libxl__xs_read(gc, t, GCSPRINTF("%s/%s/backend-id", fe_path, dir));
> + /* FIXME what if xenstore is broken? */
> + if (tmp)
> + ctrl->backend_domid = atoi(tmp);
> +
> + parsed_ok = false;
> + tmp = libxl__xs_read(gc, t, GCSPRINTF("%s/feature-host", be_path));
> + if (tmp)
> + parsed_ok = atoi(tmp) != 0;
> + libxl_defbool_set(&ctrl->scsi_raw_cmds, parsed_ok);
> +
> + parsed_ok = true;
> + devs_path = GCSPRINTF("%s/vscsi-devs", be_path);
> + dev_dirs = libxl__xs_directory(gc, t, devs_path, &ndev_dirs);
> + for (dev_dir = 0; dev_dirs && dev_dir < ndev_dirs; dev_dir++) {
> + libxl_device_vscsidev_init(&dev);
> + parsed_ok = libxl__vscsi_fill_dev(gc, t, devs_path,
> dev_dirs[dev_dir], &dev);
> + if (parsed_ok == true)
> + libxl_device_vscsictrl_append_vscsidev(CTX, ctrl, &dev);
> + libxl_device_vscsidev_dispose(&dev);
> + if (parsed_ok == false)
> + break;
> + }
> +
> + return parsed_ok;
> +}
> +
> +int libxl__vscsi_collect_ctrls(libxl__gc *gc,
> + uint32_t domid,
> + libxl_device_vscsictrl **ctrls,
> + int *num)
> +{
> + xs_transaction_t t = XBT_NULL;
> + libxl_device_vscsictrl ctrl;
> + char *fe_path;
Hard tab here.
[...]
> +static void libxl__device_vscsidev_rm(libxl__egc *egc,
> + libxl__ao_device *aodev)
> +{
> + STATE_AO_GC(aodev->ao);
> + libxl__vscsidev_rm *vscsidev_rm = CONTAINER_OF(aodev, *vscsidev_rm,
> aodev);
> + char *state_path;
> + int rc, be_wait;
> +
> + vscsidev_rm->be_path = libxl__device_backend_path(gc, aodev->dev);
> + state_path = GCSPRINTF("%s/state", vscsidev_rm->be_path);
> +
> + rc = libxl__device_vscsi_reconfigure_rm(aodev, state_path, &be_wait);
> + if (rc) goto out;
> +
> + rc = libxl__ev_devstate_wait(ao, &aodev->backend_ds,
> + libxl__device_vscsidev_rm_be,
> + state_path, be_wait,
> + LIBXL_INIT_TIMEOUT * 1000);
Should be LIBXL_DESTROY_TIMEOUT?
> + if (rc) {
> + LOG(ERROR, "unable to wait for %s", state_path);
> + goto out;
> + }
> +
> + return;
> +
> +out:
> + aodev->rc = rc;
> + /* Notify that this is done */
> + aodev->callback(egc, aodev);
> +}
> +
[...]
> diff --git a/tools/libxl/libxlu_vscsi.c b/tools/libxl/libxlu_vscsi.c
I skipped this for now. But please be aware libxlu functions are
considered to be stable API, so please don't expose more functions than
necessary.
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index bdab125..877c695 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -89,6 +89,9 @@ int main_channellist(int argc, char **argv);
> int main_blockattach(int argc, char **argv);
> int main_blocklist(int argc, char **argv);
> int main_blockdetach(int argc, char **argv);
> +int main_vscsiattach(int argc, char **argv);
> +int main_vscsilist(int argc, char **argv);
> +int main_vscsidetach(int argc, char **argv);
What about other commands? Looking at PVUSB series:
+int main_usbctrl_attach(int argc, char **argv);
+int main_usbctrl_detach(int argc, char **argv);
+int main_usbdev_attach(int argc, char **argv);
+int main_usbdev_detach(int argc, char **argv);
+int main_usblist(int argc, char **argv);
so we should be able to attach / detach both controller and specific
device?
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |