Re: [Xen-devel] [PATCH 3 of 5 RFC] blktap3: Introduce xenio.c, core xenio daemon functionality

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-11-28 at 14:20 +0000, Thanos Makatos wrote:

> +/*
> + * XenStore path components.
> + *
> + * TODO "xenio" is defined in the IDL, take it from there instead of
> + * hard-coding it here

Is there an IDL somewhere that I've failed to find?

> + */
> +#define XENIO_BACKEND_NAME     "xenio"
> +#define XENIO_BACKEND_PATH     "backend/"XENIO_BACKEND_NAME
> +#define XENIO_BACKEND_TOKEN    "backend-"XENIO_BACKEND_NAME
> +
[...]
> +/**
> + * Reads the specified XenStore path. The caller must free the returned 
> buffer.
> + *
> + * @param xs handle to XenStore
> + * @param xst XenStore transaction (TODO Maybe NULL?)
> + * @param fmt TODO
> + * @param ap TODO
> + * @returns TODO
> + *
> + * TODO Why don't we return the data pointer?
> + */
> +static char *
> +xenio_xs_vread(struct xs_handle * const xs, xs_transaction_t xst,
> +        const char * const fmt, va_list ap)
> +{
> +    char *path, *data, *s = NULL;
> +    unsigned int len;
> +
> +    assert(xs);
> +
> +    path = vmprintf(fmt, ap);
> +    data = xs_read(xs, xst, path, &len);
> +    DBG("XS read %s -> %s \n", path, data);
> +    free(path);
> +
> +    if (data) {
> +        s = strndup(data, len);
> +        free(data);

Is this a rather complicated way of ensuring the string is NULL
terminated?

I suppose given the xs protocol and/or libxenstore doesn't necessarily
NULL terminate the data or leave enough room in the allocated buffer to
add a NULL this might actually be required, yuk!

> +    }
> +
> +    return s;
> +}
> +
[...]
> +    /*
> +     * Set a watch on the back-end path using a token.
> +     *
> +     * TODO Do we really need to supply a token, given that this is the 
> _only_
> +     * watch on this specific path by this process?

xenio_backend_read_watch appears to use the token to distinguish the two
cases of watch which you have?

> +     */
> +    nerr = xs_watch(backend.xs, XENIO_BACKEND_PATH, XENIO_BACKEND_TOKEN);
> +    if (!nerr) {
> +        err = -errno;
> +        goto fail;
> +    }
> +
> +    backend.ops = ops;
> +
> +    return 0;
> +
> +fail:
> +       xenio_backend_destroy();
> +
> +    return -err;
> +}

There's obviously a lot of code here, I'm not going to pretend I read it
all, but I did have a skim and commented on what leapt out.

If there are any areas which you think could benefit from closer review,
e.g. bits which are security sensitive or where you are unsure about the
compatibility requirements for various guests or something like that
then please do highlight them and I can take a closer look.

Some of your TODOs ask more general questions about Xen PV protocols etc
which I suspect could be pretty quickly by someone on this list who is
already familiar with that niche if you posted them as questions rather
than part of the patch -- please don't feel you have to reverse engineer
the whole thing yourself ;-)

Ian.


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