Re: [Xen-devel] [PATCH v5 for-4.5 2/2] xl: add support for 'channels'

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

David Scott writes ("[PATCH v5 for-4.5 2/2] xl: add support for 'channels'"):
> This adds support for channel declarations of the form:

This looks pretty good to me.  I have some small complaints:

> +Each B<CHANNEL_SPEC_STRING> is a comma-separated list of C<KEY=VALUE>
> +settings, from the following list:

You should specify that spaces are stripped, and where.

You should also probably mention that there is no way to include a
comma, or AFAICT leading/trailing whitespace, in any of the values of
these fields.

> +static char *xstrdup(const char *x)
> +{
> +    char *r;
> +    r = strdup(x);

Breaking this kind of thing out into a separate patch would make
review a bit easier.  This applies the movement of replace_string and
arguably to the introduction of the trim and split functions.

> +typedef int (*char_predicate_t)(const int c);

You need a comment here about the bizarre interface to ctype.h macros.
See the comment by the CTYPE macro in libxl_internal.h.

> +static void trim(char_predicate_t predicate, const char *input, char 
> **output)
...
> +    while ((*p != '\000') && (predicate(*p)))

And these calls to predicate() are wrong because they don't take
account of the ctype API bug.  (Please don't ask why they left this
ridiculous landmine here...)

I haven't reviewed your string fiddling in detail.  It would probaby
be good for someone to do so because this kind of thing can contain
security bugs (which are relevant if anyone constructs an xl domain
config out of laundered pieces some of which are supplied by an
attacker).

> +int main_channellist(int argc, char **argv)
> +{

What is the way to get the channel list in machine-readable JSON
format ?

Thanks,
Ian.

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