Re: [Xen-devel] [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
On Tue, Dec 08, 2015 at 05:15:28PM +, Ian Campbell wrote: > On Tue, 2015-12-01 at 11:53 +, George Dunlap wrote: > > Return proper error codes on failure so that scripts can tell whether > > the command completed properly or not. > > > > Also while we're at it, normalize init and dispose of > > libxl_device_disk structures. This means calling init and dispose in > > the actual functions where they are declared. > > > > This in turn means changing parse_disk_config_multistring() to not > > call libxl_device_disk_init. And that in turn means changes to > > callers of parse_disk_config(). > > > > It also means removing libxl_device_disk_init() from > > libxl.c:libxl_vdev_to_device_disk(). This is only called from > > xl_cmdimpl.c. > > ... and the ocaml bindings. > > I can't remember what we decided regarding libxl "getter" functions and > initialisation of the data type (i.e. whose responsibility it is), but it > seems that changing a given calls semantics is rather dangerous API-wise. > > Wei, ISTR you stumbling over this once and the formulation of A Plan(tm), > but I can't remember what it was, can you? > I vaguely remember getting into something about libxl_device_disk_init when I was discussing with with Jim regarding libvirt, but I can't remember exactly what. It's probably a different issue. Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
On 08/12/15 17:15, Ian Campbell wrote: > On Tue, 2015-12-01 at 11:53 +, George Dunlap wrote: >> Return proper error codes on failure so that scripts can tell whether >> the command completed properly or not. >> >> Also while we're at it, normalize init and dispose of >> libxl_device_disk structures. This means calling init and dispose in >> the actual functions where they are declared. >> >> This in turn means changing parse_disk_config_multistring() to not >> call libxl_device_disk_init. And that in turn means changes to >> callers of parse_disk_config(). >> >> It also means removing libxl_device_disk_init() from >> libxl.c:libxl_vdev_to_device_disk(). This is only called from >> xl_cmdimpl.c. > > ... and the ocaml bindings. > > I can't remember what we decided regarding libxl "getter" functions and > initialisation of the data type (i.e. whose responsibility it is), but it > seems that changing a given calls semantics is rather dangerous API-wise. Right -- looks like there are similar issues with the nic lookup routines as well. I'll drop that bit from the patch. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
On 16/12/15 16:53, George Dunlap wrote: > On 08/12/15 17:15, Ian Campbell wrote: >> On Tue, 2015-12-01 at 11:53 +, George Dunlap wrote: >>> Return proper error codes on failure so that scripts can tell whether >>> the command completed properly or not. >>> >>> Also while we're at it, normalize init and dispose of >>> libxl_device_disk structures. This means calling init and dispose in >>> the actual functions where they are declared. >>> >>> This in turn means changing parse_disk_config_multistring() to not >>> call libxl_device_disk_init. And that in turn means changes to >>> callers of parse_disk_config(). >>> >>> It also means removing libxl_device_disk_init() from >>> libxl.c:libxl_vdev_to_device_disk(). This is only called from >>> xl_cmdimpl.c. >> >> ... and the ocaml bindings. >> >> I can't remember what we decided regarding libxl "getter" functions and >> initialisation of the data type (i.e. whose responsibility it is), but it >> seems that changing a given calls semantics is rather dangerous API-wise. > > Right -- looks like there are similar issues with the nic lookup > routines as well. I'll drop that bit from the patch. Hmm -- and upon further inspection, it appears that the headline feature (returning appropriate error codes) was already checked in. What was left was only the "while we're at it" bit. :-/ I'll drop this from the series... -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
On Tue, Dec 1, 2015 at 11:53 AM, George Dunlapwrote: > Return proper error codes on failure so that scripts can tell whether > the command completed properly or not. > > Also while we're at it, normalize init and dispose of > libxl_device_disk structures. This means calling init and dispose in > the actual functions where they are declared. > > This in turn means changing parse_disk_config_multistring() to not > call libxl_device_disk_init. And that in turn means changes to > callers of parse_disk_config(). > > It also means removing libxl_device_disk_init() from > libxl.c:libxl_vdev_to_device_disk(). This is only called from > xl_cmdimpl.c. > > Signed-off-by: George Dunlap Ping? -George > --- > CC: Ian Campbell > CC: Ian Jackson > CC: Wei Liu > CC: Konrad Wilk > > v3: > - Rebased to tip > > v2: > - Restructure functions to make sure init and dispose are properly > called. > --- > tools/libxl/libxl.c | 2 -- > tools/libxl/xl_cmdimpl.c | 29 - > 2 files changed, 20 insertions(+), 11 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index bd3aac8..814d056 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -2738,8 +2738,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t > domid, > if (devid < 0) > return ERROR_INVAL; > > -libxl_device_disk_init(disk); > - > dompath = libxl__xs_get_dompath(gc, domid); > if (!dompath) { > goto out; > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 2b6371d..2ba2393 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -570,8 +570,6 @@ static void parse_disk_config_multistring(XLU_Config > **config, > { > int e; > > -libxl_device_disk_init(disk); > - > if (!*config) { > *config = xlu_cfg_init(stderr, "command line"); > if (!*config) { perror("xlu_cfg_init"); exit(-1); } > @@ -3340,6 +3338,8 @@ static int cd_insert(uint32_t domid, const char > *virtdev, char *phys) > xasprintf(, "vdev=%s,access=r,devtype=cdrom,target=%s", >virtdev, phys ? phys : ""); > > +libxl_device_disk_init(); > + > parse_disk_config(, buf, ); > > /* ATM the existence of the backing file is not checked for qdisk > @@ -6649,6 +6649,7 @@ int main_blockattach(int argc, char **argv) > uint32_t fe_domid; > libxl_device_disk disk; > XLU_Config *config = 0; > +int rc = 0; > > SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) { > /* No options */ > @@ -6660,6 +6661,8 @@ int main_blockattach(int argc, char **argv) > } > optind++; > > +libxl_device_disk_init(); > + > parse_disk_config_multistring > (, argc-optind, (const char* const*)argv + optind, ); > > @@ -6668,14 +6671,17 @@ int main_blockattach(int argc, char **argv) > printf("disk: %s\n", json); > free(json); > if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); } > -return 0; > +goto out; > } > > if (libxl_device_disk_add(ctx, fe_domid, , 0)) { > fprintf(stderr, "libxl_device_disk_add failed.\n"); > -return 1; > +rc = 1; > } > -return 0; > +out: > +libxl_device_disk_dispose(); > + > +return rc; > } > > int main_blocklist(int argc, char **argv) > @@ -6726,18 +6732,23 @@ int main_blockdetach(int argc, char **argv) > /* No options */ > } > > +libxl_device_disk_init(); > + > domid = find_domain(argv[optind]); > > if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], )) { > fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]); > -return 1; > +rc = 1; > +goto out; > } > -rc = libxl_device_disk_remove(ctx, domid, , 0); > -if (rc) { > +if (libxl_device_disk_remove(ctx, domid, , 0)) { > fprintf(stderr, "libxl_device_disk_remove failed.\n"); > -return 1; > +rc = 1; > } > + > +out: > libxl_device_disk_dispose(); > + > return rc; > } > > -- > 2.1.4 > > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
On Tue, 2015-12-01 at 11:53 +, George Dunlap wrote: > Return proper error codes on failure so that scripts can tell whether > the command completed properly or not. > > Also while we're at it, normalize init and dispose of > libxl_device_disk structures. This means calling init and dispose in > the actual functions where they are declared. > > This in turn means changing parse_disk_config_multistring() to not > call libxl_device_disk_init. And that in turn means changes to > callers of parse_disk_config(). > > It also means removing libxl_device_disk_init() from > libxl.c:libxl_vdev_to_device_disk(). This is only called from > xl_cmdimpl.c. ... and the ocaml bindings. I can't remember what we decided regarding libxl "getter" functions and initialisation of the data type (i.e. whose responsibility it is), but it seems that changing a given calls semantics is rather dangerous API-wise. Wei, ISTR you stumbling over this once and the formulation of A Plan(tm), but I can't remember what it was, can you? > > Signed-off-by: George Dunlap> --- > CC: Ian Campbell > CC: Ian Jackson > CC: Wei Liu > CC: Konrad Wilk > > v3: > - Rebased to tip > > v2: > - Restructure functions to make sure init and dispose are properly > called. > --- > tools/libxl/libxl.c | 2 -- > tools/libxl/xl_cmdimpl.c | 29 - > 2 files changed, 20 insertions(+), 11 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index bd3aac8..814d056 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -2738,8 +2738,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, > uint32_t domid, > if (devid < 0) > return ERROR_INVAL; > > -libxl_device_disk_init(disk); > - > dompath = libxl__xs_get_dompath(gc, domid); > if (!dompath) { > goto out; > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 2b6371d..2ba2393 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -570,8 +570,6 @@ static void parse_disk_config_multistring(XLU_Config > **config, > { > int e; > > -libxl_device_disk_init(disk); > - > if (!*config) { > *config = xlu_cfg_init(stderr, "command line"); > if (!*config) { perror("xlu_cfg_init"); exit(-1); } > @@ -3340,6 +3338,8 @@ static int cd_insert(uint32_t domid, const char > *virtdev, char *phys) > xasprintf(, "vdev=%s,access=r,devtype=cdrom,target=%s", > virtdev, phys ? phys : ""); > > +libxl_device_disk_init(); > + > parse_disk_config(, buf, ); > > /* ATM the existence of the backing file is not checked for qdisk > @@ -6649,6 +6649,7 @@ int main_blockattach(int argc, char **argv) > uint32_t fe_domid; > libxl_device_disk disk; > XLU_Config *config = 0; > +int rc = 0; > > SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) { > /* No options */ > @@ -6660,6 +6661,8 @@ int main_blockattach(int argc, char **argv) > } > optind++; > > +libxl_device_disk_init(); > + > parse_disk_config_multistring > (, argc-optind, (const char* const*)argv + optind, > ); > > @@ -6668,14 +6671,17 @@ int main_blockattach(int argc, char **argv) > printf("disk: %s\n", json); > free(json); > if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(- > 1); } > -return 0; > +goto out; > } > > if (libxl_device_disk_add(ctx, fe_domid, , 0)) { > fprintf(stderr, "libxl_device_disk_add failed.\n"); > -return 1; > +rc = 1; > } > -return 0; > +out: > +libxl_device_disk_dispose(); > + > +return rc; > } > > int main_blocklist(int argc, char **argv) > @@ -6726,18 +6732,23 @@ int main_blockdetach(int argc, char **argv) > /* No options */ > } > > +libxl_device_disk_init(); > + > domid = find_domain(argv[optind]); > > if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], )) { > fprintf(stderr, "Error: Device %s not connected.\n", > argv[optind+1]); > -return 1; > +rc = 1; > +goto out; > } > -rc = libxl_device_disk_remove(ctx, domid, , 0); > -if (rc) { > +if (libxl_device_disk_remove(ctx, domid, , 0)) { > fprintf(stderr, "libxl_device_disk_remove failed.\n"); > -return 1; > +rc = 1; > } > + > +out: > libxl_device_disk_dispose(); > + > return rc; > } > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
Return proper error codes on failure so that scripts can tell whether the command completed properly or not. Also while we're at it, normalize init and dispose of libxl_device_disk structures. This means calling init and dispose in the actual functions where they are declared. This in turn means changing parse_disk_config_multistring() to not call libxl_device_disk_init. And that in turn means changes to callers of parse_disk_config(). It also means removing libxl_device_disk_init() from libxl.c:libxl_vdev_to_device_disk(). This is only called from xl_cmdimpl.c. Signed-off-by: George Dunlap--- CC: Ian Campbell CC: Ian Jackson CC: Wei Liu CC: Konrad Wilk v3: - Rebased to tip v2: - Restructure functions to make sure init and dispose are properly called. --- tools/libxl/libxl.c | 2 -- tools/libxl/xl_cmdimpl.c | 29 - 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index bd3aac8..814d056 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -2738,8 +2738,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid, if (devid < 0) return ERROR_INVAL; -libxl_device_disk_init(disk); - dompath = libxl__xs_get_dompath(gc, domid); if (!dompath) { goto out; diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 2b6371d..2ba2393 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -570,8 +570,6 @@ static void parse_disk_config_multistring(XLU_Config **config, { int e; -libxl_device_disk_init(disk); - if (!*config) { *config = xlu_cfg_init(stderr, "command line"); if (!*config) { perror("xlu_cfg_init"); exit(-1); } @@ -3340,6 +3338,8 @@ static int cd_insert(uint32_t domid, const char *virtdev, char *phys) xasprintf(, "vdev=%s,access=r,devtype=cdrom,target=%s", virtdev, phys ? phys : ""); +libxl_device_disk_init(); + parse_disk_config(, buf, ); /* ATM the existence of the backing file is not checked for qdisk @@ -6649,6 +6649,7 @@ int main_blockattach(int argc, char **argv) uint32_t fe_domid; libxl_device_disk disk; XLU_Config *config = 0; +int rc = 0; SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) { /* No options */ @@ -6660,6 +6661,8 @@ int main_blockattach(int argc, char **argv) } optind++; +libxl_device_disk_init(); + parse_disk_config_multistring (, argc-optind, (const char* const*)argv + optind, ); @@ -6668,14 +6671,17 @@ int main_blockattach(int argc, char **argv) printf("disk: %s\n", json); free(json); if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); } -return 0; +goto out; } if (libxl_device_disk_add(ctx, fe_domid, , 0)) { fprintf(stderr, "libxl_device_disk_add failed.\n"); -return 1; +rc = 1; } -return 0; +out: +libxl_device_disk_dispose(); + +return rc; } int main_blocklist(int argc, char **argv) @@ -6726,18 +6732,23 @@ int main_blockdetach(int argc, char **argv) /* No options */ } +libxl_device_disk_init(); + domid = find_domain(argv[optind]); if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], )) { fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]); -return 1; +rc = 1; +goto out; } -rc = libxl_device_disk_remove(ctx, domid, , 0); -if (rc) { +if (libxl_device_disk_remove(ctx, domid, , 0)) { fprintf(stderr, "libxl_device_disk_remove failed.\n"); -return 1; +rc = 1; } + +out: libxl_device_disk_dispose(); + return rc; } -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel