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

[Thanos Makatos <thanos.makatos@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: 29 November 2012 15:26
> To: Thanos Makatos
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH 3 of 5 RFC] blktap3: Introduce xenio.c,
> core xenio daemon functionality
> 
> 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?

Libxl uses LIBXL__DEVICE_KIND_XENIO instead of LIBXL__DEVICE_KIND_VBD to avoid 
confusion, and this is defined in tools/libxl/libxl_types_internal.idl. This 
will be introduced by the very latest patch that will add blktap3 support in 
libxl. I'll extended the comment to clarify this.

> 
> > + */
> > +#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?

Probably, I see no other logical explanation -- I'll simplify it.

> 
> 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.

I was actually thinking of splitting this file into smaller ones as it appears 
too big to review.

> 
> 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 ;-)

Many of these TODOs are questions to me where I need to understand that 
particular bit in order to gain a wider understanding of what that piece of 
code does so I can document it. I'll try to separate these TODOs from the ones 
for which I need a reviewer's help.

> 
> Ian.

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