Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 00 of 10] A bunch of fixes (and one feature) for libxl
A bunch of simple fixes for libxl. Most of them fixes SEGV or other annoying bugs. The last patch implements simple way to reuse block* scripts in /etc/xen/scripts (called by udev). This most likely will be ignored because of more elegant way pending in paches on the list. But maybe someone will be interested in it. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 01 of 10] libxl: Remove frontend and backend devices from xenstore after destroy
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306962865 -7200 # Node ID e3a3f5cc95349e92b7cb8b1448e999ffc16bd060 # Parent 43acc031eb24945973dffda2b7caf976993bbd5f libxl: Remove frontend and backend devices from xenstore after destroy Cleanup frontend and backend devices from xenstore for all dev types - not only disks. Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -1065,8 +1065,6 @@ device.devid = devid; device.kind = DEVICE_VBD; rc = libxl__device_del(ctx, &device, wait); - xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, &device)); - xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, &device)); libxl__free_all(&gc); return rc; } diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c --- a/tools/libxl/libxl_device.c +++ b/tools/libxl/libxl_device.c @@ -401,6 +401,8 @@ (void)wait_for_dev_destroy(ctx, &tv); } + xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, dev)); + xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev)); rc = 0; out: _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 02 of 10] libxl: Do not start stubdom when not needed
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306962900 -7200 # Node ID 59cff6b471f3f0c1e91349ed6653a47a40d4fd55 # Parent e3a3f5cc95349e92b7cb8b1448e999ffc16bd060 libxl: Do not start stubdom when not needed Do not start stubdom when there is any disk - only when there are disks requiring it. Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -828,8 +828,14 @@ goto out; } - if (nr_disks > 0 && !libxl__blktap_enabled(&gc)) - ret = 1; + if (nr_disks > 0 && !libxl__blktap_enabled(&gc)) { + for (i = 0; i < nr_disks; i++) { + if (disks[i].backend == DISK_BACKEND_TAP || disks[i].backend == DISK_BACKEND_QDISK) { + ret = 1; + goto out; + } + } + } out: libxl__free_all(&gc); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 03 of 10] libxl: Accept disk name in libxl_devid_to_device_disk
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306962929 -7200 # Node ID 81df382694fd6e208255197d8da19bfe435a2cbd # Parent 59cff6b471f3f0c1e91349ed6653a47a40d4fd55 libxl: Accept disk name in libxl_devid_to_device_disk Accept disk name in xl block-detach. Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c --- a/tools/libxl/libxl_utils.c +++ b/tools/libxl/libxl_utils.c @@ -529,18 +529,18 @@ const char *devid, libxl_device_disk *disk) { libxl__gc gc = LIBXL_INIT_GC(ctx); - char *endptr, *val; + char *val; char *dompath, *diskpath, *be_path; unsigned int devid_n; int rc = ERROR_INVAL; - devid_n = strtoul(devid, &endptr, 10); - if (devid == endptr) { + devid_n = libxl__device_disk_dev_number((char *)devid); + if (devid_n < 0) { goto out; } rc = ERROR_FAIL; dompath = libxl__xs_get_dompath(&gc, domid); - diskpath = libxl__sprintf(&gc, "%s/device/vbd/%s", dompath, devid); + diskpath = libxl__sprintf(&gc, "%s/device/vbd/%d", dompath, devid_n); if (!diskpath) { goto out; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 04 of 10] libxl: Allocate memory for strings in libxl_device_disk
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306962954 -7200 # Node ID df639d3eef683460b6d5ab38296cbd90b26f60f0 # Parent 81df382694fd6e208255197d8da19bfe435a2cbd libxl: Allocate memory for strings in libxl_device_disk Memory for strings in libxl_device_disk must be allocated from outside of libxl__gc to not be freed at the end of function (by libxl__free_all). Fixes xl block-detach Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c --- a/tools/libxl/libxl_utils.c +++ b/tools/libxl/libxl_utils.c @@ -551,10 +551,10 @@ disk->backend_domid = strtoul(val, NULL, 10); disk->domid = domid; be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath)); - disk->pdev_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path)); + disk->pdev_path = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path))); val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path)); libxl_string_to_backend(ctx, val, &(disk->backend)); - disk->vdev = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path)); + disk->vdev = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path))); val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path)); disk->unpluggable = !strcmp(val, "1"); val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mode", be_path)); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 05 of 10] libxl: Set libxl_device_nic->domid when looking up by devid
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306962980 -7200 # Node ID 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b # Parent df639d3eef683460b6d5ab38296cbd90b26f60f0 libxl: Set libxl_device_nic->domid when looking up by devid Fixes xl network-detach with device specified by id, not MAC Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c --- a/tools/libxl/libxl_utils.c +++ b/tools/libxl/libxl_utils.c @@ -512,6 +512,7 @@ } nic->backend_domid = strtoul(val, NULL, 10); nic->devid = strtoul(devid, NULL, 10); + nic->domid = domid; val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mac", nic_path_fe)); for (i = 0, tok = strtok(val, ":"); tok && (i < 6); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 06 of 10] xl: Allocate memory for libxl_device_nic string members
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306963069 -7200 # Node ID eb7216a75b7d7a5de93c717401b447545022b582 # Parent 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b xl: Allocate memory for libxl_device_nic string members Do not use argv directly - libxl_device_nic_destroy will try to free() it. Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -4280,12 +4280,14 @@ nic.mac[i] = val; } } else if (!strncmp("bridge=", *argv, 7)) { - nic.bridge = (*argv) + 7; + free(nic.bridge); + nic.bridge = strdup((*argv) + 7); } else if (!strncmp("ip=", *argv, 3)) { free(nic.ip); nic.ip = strdup((*argv) + 3); } else if (!strncmp("script=", *argv, 6)) { - nic.script = (*argv) + 6; + free(nic.script); + nic.script = strdup((*argv) + 6); } else if (!strncmp("backend=", *argv, 8)) { if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) { fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n"); @@ -4293,9 +4295,11 @@ } nic.backend_domid = val; } else if (!strncmp("vifname=", *argv, 8)) { - nic.ifname = (*argv) + 8; + free(nic.ifname); + nic.ifname = strdup((*argv) + 8); } else if (!strncmp("model=", *argv, 6)) { - nic.model = (*argv) + 6; + free(nic.model); + nic.model = strdup((*argv) + 6); } else if (!strncmp("rate=", *argv, 5)) { } else if (!strncmp("accel=", *argv, 6)) { } else { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 07 of 10] xl: Fix ''script'' param parsing in network-attach
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306963105 -7200 # Node ID 6811aa543e69379557ff7391ea3db8a5e7f7dde0 # Parent eb7216a75b7d7a5de93c717401b447545022b582 xl: Fix ''script'' param parsing in network-attach Fix ''script='' string length Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -4285,9 +4285,9 @@ } else if (!strncmp("ip=", *argv, 3)) { free(nic.ip); nic.ip = strdup((*argv) + 3); - } else if (!strncmp("script=", *argv, 6)) { + } else if (!strncmp("script=", *argv, 7)) { free(nic.script); - nic.script = strdup((*argv) + 6); + nic.script = strdup((*argv) + 7); } else if (!strncmp("backend=", *argv, 8)) { if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) { fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n"); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 08 of 10] xen.lowlevel.xl: Return None on empty domain name
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306963128 -7200 # Node ID d03bd7a830bff8162b6b0564d27b3f36ca52d8bb # Parent 6811aa543e69379557ff7391ea3db8a5e7f7dde0 xen.lowlevel.xl: Return None on empty domain name Previously PyString_FromString(NULL) was called, which caused assertion failure. Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c --- a/tools/python/xen/lowlevel/xl/xl.c +++ b/tools/python/xen/lowlevel/xl/xl.c @@ -418,8 +418,11 @@ return NULL; domname = libxl_domid_to_name(&self->ctx, domid); - ret = PyString_FromString(domname); - free(domname); + if (domname) { + ret = PyString_FromString(domname); + free(domname); + } else + return Py_None; return ret; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 09 of 10] libxl: Do not SEGV when no ''removable'' disk parameter in xenstore
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306963174 -7200 # Node ID 10bdf8aa5b8a1cb2178f2b9cbcc9ce2fd5644403 # Parent d03bd7a830bff8162b6b0564d27b3f36ca52d8bb libxl: Do not SEGV when no ''removable'' disk parameter in xenstore Just assume disk as not removable when no ''removable'' paremeter Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -1722,6 +1722,7 @@ libxl__xs_get_dompath(&gc, 0), type, domid); dir = libxl__xs_directory(&gc, XBT_NULL, be_path, &n); if (dir) { + char *removable; *disks = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n)); pdisk = *disks + *ndisks; *ndisks += n; @@ -1740,7 +1741,11 @@ libxl__sprintf(&gc, "%s/%s/type", be_path, *dir)), &(pdisk->backend)); pdisk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/%s/dev", be_path, *dir), &len); - pdisk->unpluggable = atoi(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/removable", be_path, *dir))); + removable = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/removable", be_path, *dir)); + if (removable) + pdisk->unpluggable = atoi(removable); + else + pdisk->unpluggable = 0; if (!strcmp(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/mode", be_path, *dir)), "w")) pdisk->readwrite = 1; else _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-02 22:35 UTC
[Xen-devel] [PATCH 10 of 10] libxl: "script:" prefix in block device description to setup vbd by hotplug scripts
# HG changeset patch # User Marek Marczykowski <marmarek@mimuw.edu.pl> # Date 1306963204 -7200 # Node ID 1e3bf3cb3944402e01c57c400a02fe4293153458 # Parent 10bdf8aa5b8a1cb2178f2b9cbcc9ce2fd5644403 libxl: "script:" prefix in block device description to setup vbd by hotplug scripts Implements old behaviour of block-attach - only write parameters to xenstore and left all work for hotplug scripts. Allows to directly reuse xen block scripts (and maybe some custom ones). Example use: xl block-attach vm "script:nbd 1.1.1.1 9999" xvdg w Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -896,6 +896,13 @@ } } + if (disk->backend == DISK_BACKEND_SCRIPT) { + delimiter = strchr(file_name, '':''); + if (!delimiter) + return ERROR_INVAL; + return 0; + } + if ( stat(file_name, &stat_buf) != 0 ) { LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", file_name); return ERROR_INVAL; @@ -965,6 +972,24 @@ device.backend_kind = DEVICE_VBD; break; + case DISK_BACKEND_SCRIPT: + { + char *delimiter; + backend_type = libxl__strdup(&gc, disk->pdev_path); + delimiter = strchr(backend_type, '':''); + if (!delimiter) { + rc = ERROR_FAIL; + goto out_free; + } + *delimiter = ''\0''; + flexarray_append(back, "params"); + flexarray_append(back, delimiter+1); + flexarray_append(back, "scripted"); + flexarray_append(back, "1"); + + device.backend_kind = DEVICE_VBD; + } + break; case DISK_BACKEND_TAP: if (libxl__blktap_enabled(&gc) && disk->format != DISK_FORMAT_EMPTY) { const char *dev = libxl__blktap_devpath(&gc, @@ -1060,7 +1085,7 @@ device.backend_domid = disk->backend_domid; device.backend_devid = devid; device.backend_kind = - (disk->backend == DISK_BACKEND_PHY) ? DEVICE_VBD : DEVICE_TAP; + (disk->backend == DISK_BACKEND_PHY || disk->backend == DISK_BACKEND_SCRIPT) ? DEVICE_VBD : DEVICE_TAP; device.domid = disk->domid; device.devid = devid; device.kind = DEVICE_VBD; diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -185,6 +185,7 @@ DISK_BACKEND_PHY, DISK_BACKEND_TAP, DISK_BACKEND_QDISK, + DISK_BACKEND_SCRIPT, } libxl_disk_backend; typedef enum { diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c --- a/tools/libxl/libxl_device.c +++ b/tools/libxl/libxl_device.c @@ -139,6 +139,7 @@ case DISK_BACKEND_QDISK: return "qdisk"; case DISK_BACKEND_TAP: return "tap"; case DISK_BACKEND_PHY: return "phy"; + case DISK_BACKEND_SCRIPT: return "script"; default: return NULL; } } diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c --- a/tools/libxl/libxl_utils.c +++ b/tools/libxl/libxl_utils.c @@ -282,6 +282,8 @@ if (!strcmp(s, "phy")) { *backend = DISK_BACKEND_PHY; + } else if (!strcmp(s, "script")) { + *backend = DISK_BACKEND_SCRIPT; } else if (!strcmp(s, "file")) { *backend = DISK_BACKEND_TAP; } else if (!strcmp(s, "tap")) { @@ -553,8 +555,13 @@ disk->domid = domid; be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath)); disk->pdev_path = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path))); - val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path)); - libxl_string_to_backend(ctx, val, &(disk->backend)); + val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/scripted", be_path)); + if (val && atoi(val)) + disk->backend = DISK_BACKEND_SCRIPT; + else { + val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path)); + libxl_string_to_backend(ctx, val, &(disk->backend)); + } disk->vdev = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path))); val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path)); disk->unpluggable = !strcmp(val, "1"); diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -481,6 +481,10 @@ state = DSTATE_PHYSPATH; disk->format = DISK_FORMAT_RAW; disk->backend = DISK_BACKEND_PHY; + }else if ( !strcmp(tok, "script") ) { + state = DSTATE_PHYSPATH; + disk->format = DISK_FORMAT_RAW; + disk->backend = DISK_BACKEND_SCRIPT; }else if ( !strcmp(tok, "file") ) { state = DSTATE_PHYSPATH; disk->format = DISK_FORMAT_RAW; @@ -4435,6 +4439,8 @@ tok = strtok(argv[optind+1], ":"); if (!strcmp(tok, "phy")) { disk.backend = DISK_BACKEND_PHY; + } else if (!strcmp(tok, "script")) { + disk.backend = DISK_BACKEND_SCRIPT; } else if (!strcmp(tok, "file")) { disk.backend = DISK_BACKEND_TAP; } else if (!strcmp(tok, "tap")) { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-03 08:03 UTC
Re: [Xen-devel] [PATCH 01 of 10] libxl: Remove frontend and backend devices from xenstore after destroy
Hi Marek, Please can you add: [diff] showfunc = True to your ~/.hgrc. It makes patches much easier to read. On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:> # HG changeset patch > # User Marek Marczykowski <marmarek@mimuw.edu.pl> > # Date 1306962865 -7200 > # Node ID e3a3f5cc95349e92b7cb8b1448e999ffc16bd060 > # Parent 43acc031eb24945973dffda2b7caf976993bbd5f > libxl: Remove frontend and backend devices from xenstore after destroy > > Cleanup frontend and backend devices from xenstore for all dev types - not only > disks. > > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>Thanks! The whole device destroy/teardown path is a bit of a twisty maze of confusingly named functions and parameters (e.g. force == !wait, destroy vs delete vs remove etc). Any attempt to try and untangle things is most welcome and I think this is a step in the right direction.> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -1065,8 +1065,6 @@ > device.devid = devid; > device.kind = DEVICE_VBD; > rc = libxl__device_del(ctx, &device, wait); > - xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, &device)); > - xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, &device)); > libxl__free_all(&gc); > return rc; > } > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c > --- a/tools/libxl/libxl_device.c > +++ b/tools/libxl/libxl_device.c > @@ -401,6 +401,8 @@ > (void)wait_for_dev_destroy(ctx, &tv); > } > > + xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, dev)); > + xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev));In the case where wait == true libxl__device_destroy will add a watch on the be path and wait_for_dev_destroy will subsequently remove the path. I think it would be better to add the xs_rm for be path to libxl__device_destroy as the else clause to the !force which adds the watch? I think this allow the other caller (libxl__devices_destroy) to be simplified too since all that mucking around with the toremove flexarray could be dropped. Ian.> rc = 0; > > out: > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-03 08:03 UTC
Re: [Xen-devel] [PATCH 02 of 10] libxl: Do not start stubdom when not needed
On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:> # HG changeset patch > # User Marek Marczykowski <marmarek@mimuw.edu.pl> > # Date 1306962900 -7200 > # Node ID 59cff6b471f3f0c1e91349ed6653a47a40d4fd55 > # Parent e3a3f5cc95349e92b7cb8b1448e999ffc16bd060 > libxl: Do not start stubdom when not needed > > Do not start stubdom when there is any disk - only when there are disks > requiring it. > > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>What tree is your series against? This same issue was fixed by 23044:d4ca456c0c25 in mid-March. In general you need to post patches against a reasonably recent xen-unstable. You can also recommend patches (either new ones or existing commits in xen-unstable) for backporting to the x.y-testing trees if you think that is necessary. I though 23044:d4ca456c0c25 had already been backported to 4.1-testing but it seems not, Keir can we do so please? Ian.> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -828,8 +828,14 @@ > goto out; > } > > - if (nr_disks > 0 && !libxl__blktap_enabled(&gc)) > - ret = 1; > + if (nr_disks > 0 && !libxl__blktap_enabled(&gc)) { > + for (i = 0; i < nr_disks; i++) { > + if (disks[i].backend == DISK_BACKEND_TAP || disks[i].backend == DISK_BACKEND_QDISK) { > + ret = 1; > + goto out; > + } > + } > + } > > out: > libxl__free_all(&gc); > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-03 08:08 UTC
Re: [Xen-devel] [PATCH 03 of 10] libxl: Accept disk name in libxl_devid_to_device_disk
On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:> # HG changeset patch > # User Marek Marczykowski <marmarek@mimuw.edu.pl> > # Date 1306962929 -7200 > # Node ID 81df382694fd6e208255197d8da19bfe435a2cbd > # Parent 59cff6b471f3f0c1e91349ed6653a47a40d4fd55 > libxl: Accept disk name in libxl_devid_to_device_disk > > Accept disk name in xl block-detach. > > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c > --- a/tools/libxl/libxl_utils.c > +++ b/tools/libxl/libxl_utils.c > @@ -529,18 +529,18 @@ > const char *devid, libxl_device_disk *disk) > { > libxl__gc gc = LIBXL_INIT_GC(ctx); > - char *endptr, *val; > + char *val; > char *dompath, *diskpath, *be_path; > unsigned int devid_n; > int rc = ERROR_INVAL; > > - devid_n = strtoul(devid, &endptr, 10); > - if (devid == endptr) { > + devid_n = libxl__device_disk_dev_number((char *)devid);This function takes 3 parameters in xen-unstable.hg so this won''t work. Please can you rebase. Also casting away the const like that is probably unsafe (it''s likely a const for a reason) and should have raised alarm bells. libxl__device_disk_dev_number should take a const, it looks like that would be a trivial change since the function doesn''t appear to actually modify the string, although I could be mistaken, in which case libxl__device_disk_dev_number needs fixing to not modify the string. Cheers, Ian.> + if (devid_n < 0) { > goto out; > } > rc = ERROR_FAIL; > dompath = libxl__xs_get_dompath(&gc, domid); > - diskpath = libxl__sprintf(&gc, "%s/device/vbd/%s", dompath, devid); > + diskpath = libxl__sprintf(&gc, "%s/device/vbd/%d", dompath, devid_n); > if (!diskpath) { > goto out; > } > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-03 08:10 UTC
Re: [Xen-devel] [PATCH 04 of 10] libxl: Allocate memory for strings in libxl_device_disk
On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:> # HG changeset patch > # User Marek Marczykowski <marmarek@mimuw.edu.pl> > # Date 1306962954 -7200 > # Node ID df639d3eef683460b6d5ab38296cbd90b26f60f0 > # Parent 81df382694fd6e208255197d8da19bfe435a2cbd > libxl: Allocate memory for strings in libxl_device_disk > > Memory for strings in libxl_device_disk must be allocated from outside of > libxl__gc to not be freed at the end of function (by libxl__free_all). > > Fixes xl block-detach > > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c > --- a/tools/libxl/libxl_utils.c > +++ b/tools/libxl/libxl_utils.c > @@ -551,10 +551,10 @@ > disk->backend_domid = strtoul(val, NULL, 10); > disk->domid = domid; > be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath)); > - disk->pdev_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path)); > + disk->pdev_path = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path)));> val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path)); > libxl_string_to_backend(ctx, val, &(disk->backend)); > - disk->vdev = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path)); > + disk->vdev = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path)));I think it is acceptable to use xs_read() directly in both these cases and avoid the need to strdup. Ian.> val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path)); > disk->unpluggable = !strcmp(val, "1"); > val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mode", be_path)); > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-03 08:11 UTC
Re: [Xen-devel] [PATCH 05 of 10] libxl: Set libxl_device_nic->domid when looking up by devid
On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:> # HG changeset patch > # User Marek Marczykowski <marmarek@mimuw.edu.pl> > # Date 1306962980 -7200 > # Node ID 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b > # Parent df639d3eef683460b6d5ab38296cbd90b26f60f0 > libxl: Set libxl_device_nic->domid when looking up by devid > > Fixes xl network-detach with device specified by id, not MACPlease rebase onto xen-unstable.hg, the libxl_device_* structures no longer contain a domid in that tree, it is passed as a parameter to the relevant functions now. Ian.> > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c > --- a/tools/libxl/libxl_utils.c > +++ b/tools/libxl/libxl_utils.c > @@ -512,6 +512,7 @@ > } > nic->backend_domid = strtoul(val, NULL, 10); > nic->devid = strtoul(devid, NULL, 10); > + nic->domid = domid; > > val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mac", nic_path_fe)); > for (i = 0, tok = strtok(val, ":"); tok && (i < 6); > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-03 08:13 UTC
Re: [Xen-devel] [PATCH 06 of 10] xl: Allocate memory for libxl_device_nic string members
On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:> # HG changeset patch > # User Marek Marczykowski <marmarek@mimuw.edu.pl> > # Date 1306963069 -7200 > # Node ID eb7216a75b7d7a5de93c717401b447545022b582 > # Parent 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b > xl: Allocate memory for libxl_device_nic string members > > Do not use argv directly - libxl_device_nic_destroy will try to free() it. > > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -4280,12 +4280,14 @@ > nic.mac[i] = val; > } > } else if (!strncmp("bridge=", *argv, 7)) { > - nic.bridge = (*argv) + 7; > + free(nic.bridge); > + nic.bridge = strdup((*argv) + 7);For parsing the cfg files (which has a similar pattern) we defined a helper, xlu_cfg_replace_string. I think a similar helper for the non xlu_cfg case (e.g. replace_string) would be good here too. Ian.> } else if (!strncmp("ip=", *argv, 3)) { > free(nic.ip); > nic.ip = strdup((*argv) + 3); > } else if (!strncmp("script=", *argv, 6)) { > - nic.script = (*argv) + 6; > + free(nic.script); > + nic.script = strdup((*argv) + 6); > } else if (!strncmp("backend=", *argv, 8)) { > if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) { > fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n"); > @@ -4293,9 +4295,11 @@ > } > nic.backend_domid = val; > } else if (!strncmp("vifname=", *argv, 8)) { > - nic.ifname = (*argv) + 8; > + free(nic.ifname); > + nic.ifname = strdup((*argv) + 8); > } else if (!strncmp("model=", *argv, 6)) { > - nic.model = (*argv) + 6; > + free(nic.model); > + nic.model = strdup((*argv) + 6); > } else if (!strncmp("rate=", *argv, 5)) { > } else if (!strncmp("accel=", *argv, 6)) { > } else { > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-03 08:16 UTC
Re: [Xen-devel] [PATCH 07 of 10] xl: Fix ''script'' param parsing in network-attach
On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:> # HG changeset patch > # User Marek Marczykowski <marmarek@mimuw.edu.pl> > # Date 1306963105 -7200 > # Node ID 6811aa543e69379557ff7391ea3db8a5e7f7dde0 > # Parent eb7216a75b7d7a5de93c717401b447545022b582 > xl: Fix ''script'' param parsing in network-attach > > Fix ''script='' string length > > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -4285,9 +4285,9 @@ > } else if (!strncmp("ip=", *argv, 3)) { > free(nic.ip); > nic.ip = strdup((*argv) + 3); > - } else if (!strncmp("script=", *argv, 6)) { > + } else if (!strncmp("script=", *argv, 7)) { > free(nic.script); > - nic.script = strdup((*argv) + 6); > + nic.script = strdup((*argv) + 7);Good catch. The pre-existing use of all those strncmp(A, *argv, open-coded-sizeof(A)) must be a source of many such errors. A helper function (or macro) is probably the way to go. Do you fancy coding that up? Ian.> } else if (!strncmp("backend=", *argv, 8)) { > if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) { > fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n"); > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-03 08:22 UTC
Re: [Xen-devel] [PATCH 08 of 10] xen.lowlevel.xl: Return None on empty domain name
On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:> # HG changeset patch > # User Marek Marczykowski <marmarek@mimuw.edu.pl> > # Date 1306963128 -7200 > # Node ID d03bd7a830bff8162b6b0564d27b3f36ca52d8bb > # Parent 6811aa543e69379557ff7391ea3db8a5e7f7dde0 > xen.lowlevel.xl: Return None on empty domain name > > Previously PyString_FromString(NULL) was called, which caused assertion > failure. > > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > > diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c > --- a/tools/python/xen/lowlevel/xl/xl.c > +++ b/tools/python/xen/lowlevel/xl/xl.c > @@ -418,8 +418,11 @@ > return NULL; > > domname = libxl_domid_to_name(&self->ctx, domid); > - ret = PyString_FromString(domname); > - free(domname); > + if (domname) { > + ret = PyString_FromString(domname); > + free(domname); > + } else > + return Py_None;Please do ret = Py_None; instead. Having a mixture the single return location and individual returns isn''t nice. Personally I''d just initialise ret to Py_None in the first place and omit the else. Ian.> > return ret; > } > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-03 08:24 UTC
Re: [Xen-devel] [PATCH 10 of 10] libxl: "script:" prefix in block device description to setup vbd by hotplug scripts
On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:> # HG changeset patch > # User Marek Marczykowski <marmarek@mimuw.edu.pl> > # Date 1306963204 -7200 > # Node ID 1e3bf3cb3944402e01c57c400a02fe4293153458 > # Parent 10bdf8aa5b8a1cb2178f2b9cbcc9ce2fd5644403 > libxl: "script:" prefix in block device description to setup vbd by hotplug scripts > > Implements old behaviour of block-attach - only write parameters to xenstore > and left all work for hotplug scripts. Allows to directly reuse xen block > scripts (and maybe some custom ones). Example use: > > xl block-attach vm "script:nbd 1.1.1.1 9999" xvdg w > > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>I didn''t review this one, I think it probably interacts badly with (or more likely is obsoleted by) Ian Jackson''s recent disk parsing cleanup series. Ian.> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -896,6 +896,13 @@ > } > } > > + if (disk->backend == DISK_BACKEND_SCRIPT) { > + delimiter = strchr(file_name, '':''); > + if (!delimiter) > + return ERROR_INVAL; > + return 0; > + } > + > if ( stat(file_name, &stat_buf) != 0 ) { > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", file_name); > return ERROR_INVAL; > @@ -965,6 +972,24 @@ > > device.backend_kind = DEVICE_VBD; > break; > + case DISK_BACKEND_SCRIPT: > + { > + char *delimiter; > + backend_type = libxl__strdup(&gc, disk->pdev_path); > + delimiter = strchr(backend_type, '':''); > + if (!delimiter) { > + rc = ERROR_FAIL; > + goto out_free; > + } > + *delimiter = ''\0''; > + flexarray_append(back, "params"); > + flexarray_append(back, delimiter+1); > + flexarray_append(back, "scripted"); > + flexarray_append(back, "1"); > + > + device.backend_kind = DEVICE_VBD; > + } > + break; > case DISK_BACKEND_TAP: > if (libxl__blktap_enabled(&gc) && disk->format != DISK_FORMAT_EMPTY) { > const char *dev = libxl__blktap_devpath(&gc, > @@ -1060,7 +1085,7 @@ > device.backend_domid = disk->backend_domid; > device.backend_devid = devid; > device.backend_kind = > - (disk->backend == DISK_BACKEND_PHY) ? DEVICE_VBD : DEVICE_TAP; > + (disk->backend == DISK_BACKEND_PHY || disk->backend == DISK_BACKEND_SCRIPT) ? DEVICE_VBD : DEVICE_TAP; > device.domid = disk->domid; > device.devid = devid; > device.kind = DEVICE_VBD; > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -185,6 +185,7 @@ > DISK_BACKEND_PHY, > DISK_BACKEND_TAP, > DISK_BACKEND_QDISK, > + DISK_BACKEND_SCRIPT, > } libxl_disk_backend; > > typedef enum { > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c > --- a/tools/libxl/libxl_device.c > +++ b/tools/libxl/libxl_device.c > @@ -139,6 +139,7 @@ > case DISK_BACKEND_QDISK: return "qdisk"; > case DISK_BACKEND_TAP: return "tap"; > case DISK_BACKEND_PHY: return "phy"; > + case DISK_BACKEND_SCRIPT: return "script"; > default: return NULL; > } > } > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c > --- a/tools/libxl/libxl_utils.c > +++ b/tools/libxl/libxl_utils.c > @@ -282,6 +282,8 @@ > > if (!strcmp(s, "phy")) { > *backend = DISK_BACKEND_PHY; > + } else if (!strcmp(s, "script")) { > + *backend = DISK_BACKEND_SCRIPT; > } else if (!strcmp(s, "file")) { > *backend = DISK_BACKEND_TAP; > } else if (!strcmp(s, "tap")) { > @@ -553,8 +555,13 @@ > disk->domid = domid; > be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath)); > disk->pdev_path = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path))); > - val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path)); > - libxl_string_to_backend(ctx, val, &(disk->backend)); > + val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/scripted", be_path)); > + if (val && atoi(val)) > + disk->backend = DISK_BACKEND_SCRIPT; > + else { > + val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path)); > + libxl_string_to_backend(ctx, val, &(disk->backend)); > + } > disk->vdev = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path))); > val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path)); > disk->unpluggable = !strcmp(val, "1"); > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -481,6 +481,10 @@ > state = DSTATE_PHYSPATH; > disk->format = DISK_FORMAT_RAW; > disk->backend = DISK_BACKEND_PHY; > + }else if ( !strcmp(tok, "script") ) { > + state = DSTATE_PHYSPATH; > + disk->format = DISK_FORMAT_RAW; > + disk->backend = DISK_BACKEND_SCRIPT; > }else if ( !strcmp(tok, "file") ) { > state = DSTATE_PHYSPATH; > disk->format = DISK_FORMAT_RAW; > @@ -4435,6 +4439,8 @@ > tok = strtok(argv[optind+1], ":"); > if (!strcmp(tok, "phy")) { > disk.backend = DISK_BACKEND_PHY; > + } else if (!strcmp(tok, "script")) { > + disk.backend = DISK_BACKEND_SCRIPT; > } else if (!strcmp(tok, "file")) { > disk.backend = DISK_BACKEND_TAP; > } else if (!strcmp(tok, "tap")) { > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-03 23:24 UTC
Re: [Xen-devel] [PATCH 05 of 10] libxl: Set libxl_device_nic->domid when looking up by devid
On 03.06.2011 10:11, Ian Campbell wrote:> On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote: >> # HG changeset patch >> # User Marek Marczykowski <marmarek@mimuw.edu.pl> >> # Date 1306962980 -7200 >> # Node ID 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b >> # Parent df639d3eef683460b6d5ab38296cbd90b26f60f0 >> libxl: Set libxl_device_nic->domid when looking up by devid >> >> Fixes xl network-detach with device specified by id, not MAC > > Please rebase onto xen-unstable.hg, the libxl_device_* structures no > longer contain a domid in that tree, it is passed as a parameter to the > relevant functions now.Ok, but in 4.1 tree still domid field exists and is used. So this should be fixed somehow (by applying this patch, or by backporting some other patches from unstable)...> > Ian. > >> >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> >> >> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c >> --- a/tools/libxl/libxl_utils.c >> +++ b/tools/libxl/libxl_utils.c >> @@ -512,6 +512,7 @@ >> } >> nic->backend_domid = strtoul(val, NULL, 10); >> nic->devid = strtoul(devid, NULL, 10); >> + nic->domid = domid; >> >> val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mac", nic_path_fe)); >> for (i = 0, tok = strtok(val, ":"); tok && (i < 6); >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel > >-- Pozdrawiam / Best Regards, Marek Marczykowski | RLU #390519 marmarek at mimuw edu pl | xmpp:marmarek at staszic waw pl _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-03 23:30 UTC
Re: [Xen-devel] [PATCH 01 of 10] libxl: Remove frontend and backend devices from xenstore after destroy
On 03.06.2011 10:03, Ian Campbell wrote:> Hi Marek, > > Please can you add: > [diff] > showfunc = True > to your ~/.hgrc. It makes patches much easier to read. > > On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote: >> # HG changeset patch >> # User Marek Marczykowski <marmarek@mimuw.edu.pl> >> # Date 1306962865 -7200 >> # Node ID e3a3f5cc95349e92b7cb8b1448e999ffc16bd060 >> # Parent 43acc031eb24945973dffda2b7caf976993bbd5f >> libxl: Remove frontend and backend devices from xenstore after destroy >> >> Cleanup frontend and backend devices from xenstore for all dev types - not only >> disks. >> >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > > Thanks! > > The whole device destroy/teardown path is a bit of a twisty maze of > confusingly named functions and parameters (e.g. force == !wait, destroy > vs delete vs remove etc). Any attempt to try and untangle things is most > welcome and I think this is a step in the right direction. > >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -1065,8 +1065,6 @@ >> device.devid = devid; >> device.kind = DEVICE_VBD; >> rc = libxl__device_del(ctx, &device, wait); >> - xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, &device)); >> - xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, &device)); >> libxl__free_all(&gc); >> return rc; >> } >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c >> --- a/tools/libxl/libxl_device.c >> +++ b/tools/libxl/libxl_device.c >> @@ -401,6 +401,8 @@ >> (void)wait_for_dev_destroy(ctx, &tv); >> } >> >> + xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, dev)); >> + xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev)); > > In the case where wait == true libxl__device_destroy will add a watch on > the be path and wait_for_dev_destroy will subsequently remove the path.Only backend one...> I think it would be better to add the xs_rm for be path to > libxl__device_destroy as the else clause to the !force which adds the > watch?Also - only backend one (libxl__device_destroy gets only backend path as parameter).> I think this allow the other caller (libxl__devices_destroy) to > be simplified too since all that mucking around with the toremove > flexarray could be dropped.What about frontend device in xenstore? In most cases only backend is removed. In wait_for_dev_destroy we don''t know frontend patch (wheel, not exactly true, but not always easy). Perhaps there should be two watches - on backend AND on frontend (with writing "5" to frontend state also?)? Or backend should be removed as you suggested, but frontend as in my patch (after wait_for_dev_destroy if was called)? -- Pozdrawiam / Best Regards, Marek Marczykowski | RLU #390519 marmarek at mimuw edu pl | xmpp:marmarek at staszic waw pl _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Marek Marczykowski
2011-Jun-03 23:49 UTC
Re: [Xen-devel] [PATCH 07 of 10] xl: Fix ''script'' param parsing in network-attach
On 03.06.2011 10:16, Ian Campbell wrote:> On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote: >> # HG changeset patch >> # User Marek Marczykowski <marmarek@mimuw.edu.pl> >> # Date 1306963105 -7200 >> # Node ID 6811aa543e69379557ff7391ea3db8a5e7f7dde0 >> # Parent eb7216a75b7d7a5de93c717401b447545022b582 >> xl: Fix ''script'' param parsing in network-attach >> >> Fix ''script='' string length >> >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> >> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -4285,9 +4285,9 @@ >> } else if (!strncmp("ip=", *argv, 3)) { >> free(nic.ip); >> nic.ip = strdup((*argv) + 3); >> - } else if (!strncmp("script=", *argv, 6)) { >> + } else if (!strncmp("script=", *argv, 7)) { >> free(nic.script); >> - nic.script = strdup((*argv) + 6); >> + nic.script = strdup((*argv) + 7); > > Good catch. > > The pre-existing use of all those > strncmp(A, *argv, open-coded-sizeof(A)) > must be a source of many such errors. A helper function (or macro) is > probably the way to go. Do you fancy coding that up?And the same size must be used in strdup offset... Maybe something like this (solving also problem in 06 patch): ----------- #define COMPARE_AND_REPLACE(pattern, dest) \ } else if (!strncmp(pattern, *argv, sizeof(pattern))) { \ free(dest); \ dest = strdup((*argv) + sizeof(pattern)); (...) COMPARE_AND_REPLACE("script=", nic.script) COMPARE_AND_REPLACE("ip=", nic.ip) --------- Looks weird because of no semicolon at the end of lines, but should works. Unfortunately cannot be used in all places (like one below)... What do you think?> > Ian. > >> } else if (!strncmp("backend=", *argv, 8)) { >> if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) { >> fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");-- Pozdrawiam / Best Regards, Marek Marczykowski | RLU #390519 marmarek at mimuw edu pl | xmpp:marmarek at staszic waw pl _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-04 06:46 UTC
Re: [Xen-devel] [PATCH 01 of 10] libxl: Remove frontend and backend devices from xenstore after destroy
On Sat, 2011-06-04 at 00:30 +0100, Marek Marczykowski wrote:> On 03.06.2011 10:03, Ian Campbell wrote: > > Hi Marek, > > > > Please can you add: > > [diff] > > showfunc = True > > to your ~/.hgrc. It makes patches much easier to read. > > > > On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote: > >> # HG changeset patch > >> # User Marek Marczykowski <marmarek@mimuw.edu.pl> > >> # Date 1306962865 -7200 > >> # Node ID e3a3f5cc95349e92b7cb8b1448e999ffc16bd060 > >> # Parent 43acc031eb24945973dffda2b7caf976993bbd5f > >> libxl: Remove frontend and backend devices from xenstore after destroy > >> > >> Cleanup frontend and backend devices from xenstore for all dev types - not only > >> disks. > >> > >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > > > > Thanks! > > > > The whole device destroy/teardown path is a bit of a twisty maze of > > confusingly named functions and parameters (e.g. force == !wait, destroy > > vs delete vs remove etc). Any attempt to try and untangle things is most > > welcome and I think this is a step in the right direction. > > > >> > >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > >> --- a/tools/libxl/libxl.c > >> +++ b/tools/libxl/libxl.c > >> @@ -1065,8 +1065,6 @@ > >> device.devid = devid; > >> device.kind = DEVICE_VBD; > >> rc = libxl__device_del(ctx, &device, wait); > >> - xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, &device)); > >> - xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, &device)); > >> libxl__free_all(&gc); > >> return rc; > >> } > >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c > >> --- a/tools/libxl/libxl_device.c > >> +++ b/tools/libxl/libxl_device.c > >> @@ -401,6 +401,8 @@ > >> (void)wait_for_dev_destroy(ctx, &tv); > >> } > >> > >> + xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, dev)); > >> + xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev)); > > > > In the case where wait == true libxl__device_destroy will add a watch on > > the be path and wait_for_dev_destroy will subsequently remove the path. > > Only backend one... > > > I think it would be better to add the xs_rm for be path to > > libxl__device_destroy as the else clause to the !force which adds the > > watch? > > Also - only backend one (libxl__device_destroy gets only backend path as > parameter). > > > I think this allow the other caller (libxl__devices_destroy) to > > be simplified too since all that mucking around with the toremove > > flexarray could be dropped. > > What about frontend device in xenstore? In most cases only backend is > removed. In wait_for_dev_destroy we don''t know frontend patch (wheel, > not exactly true, but not always easy). Perhaps there should be two > watches - on backend AND on frontend (with writing "5" to frontend state > also?)? Or backend should be removed as you suggested, but frontend as > in my patch (after wait_for_dev_destroy if was called)?My comments were only about the location of the rm of the backend path. I think your original patch moved the rm of the frontend path to the right place. an. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-04 06:47 UTC
Re: [Xen-devel] [PATCH 05 of 10] libxl: Set libxl_device_nic->domid when looking up by devid
On Sat, 2011-06-04 at 00:24 +0100, Marek Marczykowski wrote:> On 03.06.2011 10:11, Ian Campbell wrote: > > On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote: > >> # HG changeset patch > >> # User Marek Marczykowski <marmarek@mimuw.edu.pl> > >> # Date 1306962980 -7200 > >> # Node ID 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b > >> # Parent df639d3eef683460b6d5ab38296cbd90b26f60f0 > >> libxl: Set libxl_device_nic->domid when looking up by devid > >> > >> Fixes xl network-detach with device specified by id, not MAC > > > > Please rebase onto xen-unstable.hg, the libxl_device_* structures no > > longer contain a domid in that tree, it is passed as a parameter to the > > relevant functions now. > > Ok, but in 4.1 tree still domid field exists and is used. So this should > be fixed somehow (by applying this patch, or by backporting some other > patches from unstable)...IMHO the patch which removes domid is not suitable for backporting (since it changes libxl API). So this patch seems suitable for 4.1 directly, I think. Thanks,Ian.> > > > > Ian. > > > >> > >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > >> > >> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c > >> --- a/tools/libxl/libxl_utils.c > >> +++ b/tools/libxl/libxl_utils.c > >> @@ -512,6 +512,7 @@ > >> } > >> nic->backend_domid = strtoul(val, NULL, 10); > >> nic->devid = strtoul(devid, NULL, 10); > >> + nic->domid = domid; > >> > >> val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mac", nic_path_fe)); > >> for (i = 0, tok = strtok(val, ":"); tok && (i < 6); > >> > >> > >> > >> _______________________________________________ > >> Xen-devel mailing list > >> Xen-devel@lists.xensource.com > >> http://lists.xensource.com/xen-devel > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-04 08:25 UTC
Re: [Xen-devel] [PATCH 07 of 10] xl: Fix ''script'' param parsing in network-attach
On Sat, 2011-06-04 at 00:49 +0100, Marek Marczykowski wrote:> On 03.06.2011 10:16, Ian Campbell wrote: > > On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote: > >> # HG changeset patch > >> # User Marek Marczykowski <marmarek@mimuw.edu.pl> > >> # Date 1306963105 -7200 > >> # Node ID 6811aa543e69379557ff7391ea3db8a5e7f7dde0 > >> # Parent eb7216a75b7d7a5de93c717401b447545022b582 > >> xl: Fix ''script'' param parsing in network-attach > >> > >> Fix ''script='' string length > >> > >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl> > >> > >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > >> --- a/tools/libxl/xl_cmdimpl.c > >> +++ b/tools/libxl/xl_cmdimpl.c > >> @@ -4285,9 +4285,9 @@ > >> } else if (!strncmp("ip=", *argv, 3)) { > >> free(nic.ip); > >> nic.ip = strdup((*argv) + 3); > >> - } else if (!strncmp("script=", *argv, 6)) { > >> + } else if (!strncmp("script=", *argv, 7)) { > >> free(nic.script); > >> - nic.script = strdup((*argv) + 6); > >> + nic.script = strdup((*argv) + 7); > > > > Good catch. > > > > The pre-existing use of all those > > strncmp(A, *argv, open-coded-sizeof(A)) > > must be a source of many such errors. A helper function (or macro) is > > probably the way to go. Do you fancy coding that up? > > And the same size must be used in strdup offset... > Maybe something like this (solving also problem in 06 patch): > ----------- > #define COMPARE_AND_REPLACE(pattern, dest) \ > } else if (!strncmp(pattern, *argv, sizeof(pattern))) { \ > free(dest); \ > dest = strdup((*argv) + sizeof(pattern)); > > (...) > COMPARE_AND_REPLACE("script=", nic.script) > COMPARE_AND_REPLACE("ip=", nic.ip) > --------- > Looks weird because of no semicolon at the end of lines, but should > works.Generally you can fix the semi-colon thing by omitting the last one from the macro itself.> Unfortunately cannot be used in all places (like one below)... > What do you think?How about (untested), requires a local "char *oparg": static int match_option_size(const char *prefix, size_t len, char *arg, char **argopt) { int rc = strncmp(prefix, argv, len); if (!rc) *argopt = argv+len; return !rc } #define match_option(_prefix, _arg, _oparg) \ match_option_size((_prefix), (_arg), sizeof((_prefix)), &(_oparg)); static void replace_string(char **str, const char *val) { free(*str);\ *str = strdup(val); } then you have: else if (match_option("script=", *argv, oparg)) replace_string(&nic.script, oparg); else if (match_option("backend=", *argv, oparg)) { if(libxl_name_to_domid(&ctx, oparg, &val)) { fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n"); ... } } with a bit of macro pasting in match_option you can probably push the "=" down into the macro so each callsite doesn''t need it. Ian.> > > > > Ian. > > > >> } else if (!strncmp("backend=", *argv, 8)) { > >> if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) { > >> fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n"); > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel