This is a very big patch, I've only skimmed it so far.
I think you need to fix some overly long lines. I won't mention them
individually inline.
Regarding all the parsing stuffs, you haven't defined vscsispec so I
cannot review it. You might want to look at
docs/misc/xl-disk-configuration.txt.
I'm not suggesting you have to write something like that, but
considering all the compatibility issues you might have you might
actually end up writing up a document like that.
Then after that, do you consider writing a lexer for vscsispec?
We have one for diskspec, see libxlu_disk_l.l. That might result in a
shorter patch?
On Fri, Mar 06, 2015 at 10:45:56AM +0100, Olaf Hering wrote:
> Port pvscsi support from xend to libxl. See pvscsi.txt for details.
>
> Outstanding work is listed in the TODO section.
>
There is no TODO section in this patch. :-)
> Signed-off-by: Olaf Hering <o...@aepfle.de>
> Cc: Ian Jackson <ian.jack...@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
> Cc: Ian Campbell <ian.campb...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>
> ---
> tools/libxl/Makefile | 1 +
> tools/libxl/libxl.c | 172 ++++++++++++++++
> tools/libxl/libxl.h | 29 +++
> tools/libxl/libxl_create.c | 1 +
> tools/libxl/libxl_device.c | 2 +
> tools/libxl/libxl_freebsd.c | 8 +
> tools/libxl/libxl_internal.h | 12 ++
> tools/libxl/libxl_linux.c | 60 ++++++
> tools/libxl/libxl_netbsd.c | 8 +
> tools/libxl/libxl_types.idl | 49 +++++
> tools/libxl/libxl_types_internal.idl | 1 +
> tools/libxl/libxl_vscsi.c | 375
> +++++++++++++++++++++++++++++++++++
> tools/libxl/xl.h | 3 +
> tools/libxl/xl_cmdimpl.c | 249 ++++++++++++++++++++++-
> tools/libxl/xl_cmdtable.c | 15 ++
Better split this patch into two. One for libxl and one for xl.
Or you can even split it into three, one for introducing libxl types,
one for implementing functionalities in libxl and one for xl.
> 15 files changed, 984 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 7329521..9622d66 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 \
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 088786e..73504b9 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1967,6 +1967,166 @@ static int libxl__resolve_domid(libxl__gc *gc, const
> char *name,
> }
>
>
> /******************************************************************************/
> +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)
You need to update this domain's JSON configuration. Cf.
libxl__device_vtpm_add and friends. Also look at libxl_internal.h L2310.
> +{
> + STATE_AO_GC(aodev->ao);
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + flexarray_t *front;
> + flexarray_t *back;
> + libxl__device *device;
> + char *be_path;
> + unsigned int be_dirs = 0, rc, i;
> +
> + if (vscsi->devid == -1) {
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + /* 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);
> +
> + GCNEW(device);
> + rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
> + if ( rc != 0 ) goto out;
> +
Coding style. No space after ( and before ). You can even just use
if (!rc) goto out;
> + /* Check if backend device path is already present */
> + be_path = libxl__device_backend_path(gc, device);
> + if (!libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs) || !be_dirs) {
> + /* backend does not exist, create a new one */
> + flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> + flexarray_append_pair(back, "online", "1");
> + flexarray_append_pair(back, "state", "1");
> + flexarray_append_pair(back, "feature-host", GCSPRINTF("%d",
> !!vscsi->feature_host));
> +
> + flexarray_append_pair(front, "backend-id", GCSPRINTF("%d",
> vscsi->backend_domid));
> + flexarray_append_pair(front, "state", "1");
> + }
> +
> + for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> + libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
> + /* Trigger removal, otherwise create new device */
Why do you want to trigger removal in "add" function?
> + if (be_dirs) {
> + unsigned int nb = 0;
> + /* Preserve existing device */
> + if (libxl__xs_directory(gc, XBT_NULL,
> GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id), &nb) && nb) {
> + /* Trigger device removal by forwarding state to
> XenbusStateClosing */
> + if (v->remove)
> + flexarray_append_pair(back,
> GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), "5");
> + continue;
> + }
> + }
> + flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-devname",
> v->vscsi_dev_id), v->p_devname);
> + switch (v->pdev_type) {
> + case LIBXL_VSCSI_PDEV_TYPE_WWN:
[...]
> + xs_transaction_t t;
> +retry_transaction:
> + t = xs_transaction_start(ctx->xsh);
> + libxl__xs_writev(gc, t, be_path,
> + libxl__xs_kvs_of_flexarray(gc, back, back->count));
> + xs_write(ctx->xsh, t, GCSPRINTF("%s/state", be_path), "7", 2);
> + if (!xs_transaction_end(ctx->xsh, t, 0)) {
> + if (errno == EAGAIN)
> + goto retry_transaction;
> + LOGE(ERROR, "xs transaction failed");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + libxl__wait_for_backend(gc, be_path, "4");
> +
> +retry_transaction2:
> + t = xs_transaction_start(ctx->xsh);
> + for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> + libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
> + if (v->remove) {
> + char *path, *val;
> + path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path,
> v->vscsi_dev_id);
> + val = libxl__xs_read(gc, t, path);
> + if (val && strcmp(val, "6") == 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);
> + } else {
> + LOGE(ERROR, "%s: %s has %s, expected 6", __func__, path,
> val);
> + }
> + }
> + }
> +
Please avoid using goto to retry transaction.
> + if (!xs_transaction_end(ctx->xsh, t, 0)) {
> + if (errno == EAGAIN)
> + goto retry_transaction2;
> + LOGE(ERROR, "xs transaction failed");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + /* As we are not adding new device, skip waiting for it */
> + libxl__ao_complete(egc, aodev->ao, 0);
> +
> + rc = 0;
> +out:
> + aodev->rc = rc;
> + if(rc) aodev->callback(egc, aodev);
> + return;
> +}
> +
[...]
> + GC_FREE;
> + return rc;
> +}
> +
> +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> + libxl_vscsi_dev *dev)
> +{
Can this only be an internal function? I.e. use libxl__ namespace.
Same comment applies to similar functions.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
