On Tue, 2015-12-01 at 11:53 +0000, 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 <george.dun...@eu.citrix.com>
> ---
> CC: Ian Campbell <ian.campb...@citrix.com>
> CC: Ian Jackson <ian.jack...@citrix.com>
> CC: Wei Liu <wei.l...@citrix.com>
> CC: Konrad Wilk <konrad.w...@oracle.com>
>
> 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(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
> virtdev, phys ? phys : "");
>
> + libxl_device_disk_init(&disk);
> +
> parse_disk_config(&config, buf, &disk);
>
> /* 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(&disk);
> +
> parse_disk_config_multistring
> (&config, argc-optind, (const char* const*)argv + optind,
> &disk);
>
> @@ -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, &disk, 0)) {
> fprintf(stderr, "libxl_device_disk_add failed.\n");
> - return 1;
> + rc = 1;
> }
> - return 0;
> +out:
> + libxl_device_disk_dispose(&disk);
> +
> + 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(&disk);
> +
> domid = find_domain(argv[optind]);
>
> if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], &disk)) {
> fprintf(stderr, "Error: Device %s not connected.\n",
> argv[optind+1]);
> - return 1;
> + rc = 1;
> + goto out;
> }
> - rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
> - if (rc) {
> + if (libxl_device_disk_remove(ctx, domid, &disk, 0)) {
> fprintf(stderr, "libxl_device_disk_remove failed.\n");
> - return 1;
> + rc = 1;
> }
> +
> +out:
> libxl_device_disk_dispose(&disk);
> +
> return rc;
> }
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
