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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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.dunlap@xxxxxxxxxxxxx>
> ---
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Konrad Wilk <konrad.wilk@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel