Re: [PATCH v3 3/3] xl: add support for channels

[Dave Scott <Dave.Scott@citrix.com>]

On 2 Jul 2014, at 16:33, Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Mon, 2014-06-23 at 15:57 +0100, Ian Jackson wrote:
>> David Scott writes ("[PATCH v3 3/3] xl: add support for channels"):
>>> This adds support for channel declarations of the form:
>>>  channel = [ "name=...,kind=...[,path=...][,backend=...]" ]
>> 
>>> +    if (!xlu_cfg_get_list (config, "channel", &channels, 0, 0)) {
>>> +        d_config->num_channels = 0;
>>> +        d_config->channels = NULL;
>>> +        while ((buf = xlu_cfg_get_listitem (channels,
>>> +                d_config->num_channels)) != NULL) {
>>> +            libxl_device_channel *chn;
>>> +            char *buf2 = strdup(buf);
>>> +            char *p, *p2;
>>> +            chn = ARRAY_EXTEND_INIT(d_config->channels, d_config->num_channels,
>>> +                                    libxl_device_channel_init);
>> 
>> I appreciate that you're just following the example of the vif
>> configuration here, but I think this is rather too much open-coded
>> string handling.
> 
> FWIW I think there are a few places in xl where a precanned parser for
> strings of comma-separated key=value syntax would be quite valuable (the
> existing one is very disk specific due to the quirks for that syntax).
> 
> I don't expect Dave to write one though…

I’ve had a go at decomposing this into ‘split_string_into_string_list’, ‘trim’ and ‘split_string_into_pair’ so at least the fiddly pointer-level stuff is in shareable functions. I’ll send around the updated patch set later today.

> 
>> 
>>> +                if (!strcmp(p, "backend")) {
>>> +                    free(chn->backend_domname);
>>> +                    chn->backend_domname = strdup(p2 + 1);
>> 
>> At the very least, can we provide a macro or function or something to
>> do this ?
> 
> I think the existing replace_string already does this.

Ah yes so it does. I’ll replace my new replace function with ‘replace_string' before I send the patches :-)

Cheers,
Dave