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

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

On 22 Sep 2014, at 17:25, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:

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

Good point.

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

OK. I’ll split these out for review and make it easy to squash them back
together (if that’s desired).

> 
>> +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…)

Thanks, I’d never noticed that before!

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

‘xl list —long’ (via list_domains_details) can display the json:

$ sudo xl list --long
[
    {
        "domid": 30,
...
            "channels": [
                {
                    "devid": 0,
                    "name": "foo",
                    "connection.socket": {
                        "path": "/var/lib/libvirt/qemu/s-4-VM.agent"
                    }
                }
            ],

...

There’s no option to ‘xl channel-list’ to show them in JSON. I notice
the xl hotplug commands have a ‘dryrun’ option which dumps the JSON of
the device they would have added, but I’ve not implemented channel
hotplug (yet)

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