Re: [Xen-devel] [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach

[Wei Liu]

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

[George Dunlap]

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

[George Dunlap]

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

[George Dunlap]

On Tue, Dec 1, 2015 at 11:53 AM, 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.
>
> 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

[Ian Campbell]

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