Re: [Xen-devel] [RFC Patch v4 9/9] update libxl__device_disk_from_xs_be() to support blktap device

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-09-22 at 13:59 +0800, Wen Congyang wrote:
> If the disk backend is blktap device, we store "format:pdev_path"
> in tapdisk-params, and store "phy" in type. So use tapdisk-params
> to set libxl_device_disk instead of params and type.

This seems to be the same format as params -- can you not just
dynamically select the key name and share the parsing code?

> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
> Cc: Shriram Rajagopalan <rshriram@xxxxxxxxx>
> ---
>  tools/libxl/libxl.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index aa9345b..8c241aa 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2468,6 +2468,45 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
>          goto cleanup;
>      }
>  
> +    disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
> +
> +    /* "tapdisk-params" is only for tapdisk */
> +    tmp = xs_read(ctx->xsh, XBT_NULL,
> +                  libxl__sprintf(gc, "%s/tapdisk-params", be_path), &len);

libxl__xs_read will gc the result, which will make the error handling
easier (you strdup tmp so I think the reasons for using xs_read don't
apply). Also libxl__sprintf can be shortended with GCSPRINTF.

> +        if (disk->format != LIBXL_DISK_FORMAT_VHD &&
> +            disk->format != LIBXL_DISK_FORMAT_RAW) {
> +            LOG(ERROR, "unsupported tapdisk format: %s\n", tmp);

Hrm, if we get here then is there any reason to reject this? obviously
something wrote it etc.

This function is supposed to read the current state and return it, I'm
not sure it should be rejecting what it finds TBH, and it would simplify
things not to do so.

> +            free(tmp);
> +            goto cleanup;
> +        }
> +        free(tmp);
> +
> +        /*
> +         * The backend is tapdisk, so we store tapdev in params, and
> +         * phy in type(see device_disk_add())
> +         */
> +        disk->backend = LIBXL_DISK_BACKEND_TAP;
> +
> +        goto skip_type;

I think you want to put some all all of the following into an else
clause, not simulate that affect with a goto.

> @@ -2520,8 +2560,6 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
>      }
>      disk->is_cdrom = !strcmp(tmp, "cdrom");
>  
> -    disk->format = LIBXL_DISK_FORMAT_UNKNOWN;

Why does this need to move? Also I think UNKNOWN is the fault from
libxl_device_disk_init, so it's probably unnecessary.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel