Re: [Xen-devel] [PATCH 2 of 7 v2] blktap3/tapback: Introduces functionality required to access XenStore

[Thanos Makatos <thanos.makatos@xxxxxxxxxx>]

> On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> > This patch introduces convenience functions that read/write values
> from/to
> > XenStore.
> >
> > diff -r 0777319f0a6e -r efd3f343f7c7 tools/blktap3/tapback/xenstore.c
> > --- /dev/null       Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/blktap3/tapback/xenstore.c      Fri Jan 04 11:54:51 2013
> +0000
> > @@ -0,0 +1,188 @@
> > +/*
> > + * Copyright (C) 2012      Citrix Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301,
> > + * USA.
> > + */
> > +
> > +#include <stdarg.h>
> > +#include <stdio.h>
> > +#include <xenstore.h>
> > +#include <assert.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +
> > +#include "blktap3.h"
> > +#include "tapback.h"
> > +#include "xenstore.h"
> > +
> > +/**
> > + * Prints the supplied arguments to a buffer and returns it. The
> buffer must
> > + * be deallocated by the user.
> 
> I'm not sure how much value this function has for callers over and
> above
> just using (v)asprintf directly, likewise for mprintf.

You're right they don't add anything, I'll remove them.

> 
> > + */
> > +static char *
> > +vmprintf(const char * const fmt, va_list ap)
> > +{
> > +    char *s = NULL;
> 
> No need to init s.

Regarding initialising variables, isn't it a good programming practice to 
initialise variables even if they'll be shortly assigned a value? Won't the 
compiler easily optimise such cases? Or does it make it too cumbersome to read? 
In any case, I checked the coding style document and it doesn't specify this, 
so I think it would be a meaningful addition.

> 
> > +
> > +    if (vasprintf(&s, fmt, ap) < 0)
> > +        s = NULL;
> > +
> > +    return s;
> > +}
> > +
> > +__printf(1, 2)
> > +char *
> > +mprintf(const char * const fmt, ...)
> > +{
> > +    va_list ap;
> > +    char *s = NULL;
> 
> No need to init s.
> 
> > +
> > +    va_start(ap, fmt);
> > +    s = vmprintf(fmt, ap);
> > +    va_end(ap);
> > +
> > +    return s;
> > +}
> > +
> > +char *
> > +tapback_xs_vread(struct xs_handle * const xs, xs_transaction_t xst,
> > +        const char * const fmt, va_list ap)
> 
> This might be an interesting/useful addition to libxenstore?

Sure.

> 
> > +{
> > +    char *path = NULL, *data = NULL;
> 
> No need to init path.
> 
> > +    unsigned int len = 0;
> > +
> > +    assert(xs);
> > +
> > +    path = vmprintf(fmt, ap);
> > +    data = xs_read(xs, xst, path, &len);
> 
> This uses path without checking if it is NULL.
> 
> I think you can pass len == NULL if you don't care about the length of
> the result.

Right.

> 
> However xenstore values generally can contain nulls and are not
> necessarily NULL terminated (see docs/misc/xenstore.txt), but perhaps
> the block protocol tightens this restriction such that you can rely on
> NULL termination (at least in practice)?

Hm.. I saw this in tools/xenstore/xenstore.h and I though xs_read does end the 
string with a NULL:

/* Get the value of a single file, nul terminated.
 * Returns a malloced value: call free() on it after use.
 * len indicates length in bytes, not including terminator.
 */
void *xs_read(struct xs_handle *h, xs_transaction_t t,
              const char *path, unsigned int *len);

Am I missing something?

> 
> > +    free(path);
> > +
> > +    return data;
> > +}
> > +
> > +__printf(3, 4)
> > +char *
> > +tapback_xs_read(struct xs_handle * const xs, xs_transaction_t xst,
> > +        const char * const fmt, ...)
> > +{
> > +    va_list ap;
> > +    char *s = NULL;
> 
> No need to init s.
> 
> > +
> > +    assert(xs);
> > +
> > +    va_start(ap, fmt);
> > +    s = tapback_xs_vread(xs, xst, fmt, ap);
> > +    va_end(ap);
> > +
> > +    return s;
> > +}
> > +
> 
> > +__scanf(3, 4)
> > +int
> > +tapback_device_scanf_otherend(vbd_t * const device,
> > +        const char * const path, const char * const fmt, ...)
> > +{
> > +    va_list ap;
> > +    int n = 0;
> > +    char *s = NULL;
> > +
> > +    assert(device);
> > +    assert(path);
> > +
> > +    if (!(s = tapback_device_read_otherend(device, path)))
> > +        return -1;
> 
> Since s here is read from the frontend you probably can't trust it to
> be
> NULL terminated.
> 
> > +    va_start(ap, fmt);
> > +    n = vsscanf(s, fmt, ap);
> > +    free(s);
> > +    va_end(ap);
> > +
> > +    return n;
> > +}
> > +
> 
> Ian.

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