[libvirt] [PATCH v1 07/14] conf: Parse user supplied aliases

[Michal Privoznik]

On 10/19/2017 11:55 AM, Martin Kletzander wrote:
> On Thu, Oct 19, 2017 at 10:11:02AM +0200, Michal Privoznik wrote:
>> If driver that is calling the parse supports user supplied
>> aliases, they can be parsed even for inactive XMLs. However, to
>> avoid any clashes with aliases that libvirt generates, the user
>> ones have to have "ua-" prefix.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> src/conf/domain_conf.c | 13 +++++++++++--
>> src/conf/domain_conf.h |  1 +
>> 2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index f4de4e288..f1386c116 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6398,6 +6398,10 @@ virDomainDeviceAddressParseXML(xmlNodePtr address,
>> }
>>
>>
>> +#define USER_ALIAS_PREFIX "ua-"
>> +#define USER_ALIAS_CHARS \
>> +    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-"
>> +
>> /* Parse the XML definition for a device address
>>  * @param node XML nodeset to parse for device address definition
>>  */
>> @@ -6424,7 +6428,6 @@
>> virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt
>> ATTRIBUTE_UNUSED,
>>     while (cur != NULL) {
>>         if (cur->type == XML_ELEMENT_NODE) {
>>             if (alias == NULL &&
>> -                !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
>>                 virXMLNodeNameEqual(cur, "alias")) {
>>                 alias = cur;
>>             } else if (address == NULL &&
>> @@ -6446,8 +6449,14 @@
>> virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt
>> ATTRIBUTE_UNUSED,
>>         cur = cur->next;
>>     }
>>
>> -    if (alias)
>> +    if (alias) {
>>         info->alias = virXMLPropString(alias, "name");
>> +        if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE &&
>> +            (!STRPREFIX(info->alias, USER_ALIAS_PREFIX) ||
>> +             !(xmlopt->config.features &
>> VIR_DOMAIN_DEF_FEATURE_USER_ALIAS) ||
>> +             strspn(info->alias, USER_ALIAS_CHARS) <
>> strlen(info->alias)))
>> +            VIR_FREE(info->alias);
> 
> Assigning the pointer just so you can clear it after some hard to read
> condition
> seems just plain wrong.  Only assign it if the condition is false.
> 
>> +    }
>>
>>     if (master) {
>>         info->mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 1e007346f..5ff85057f 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2508,6 +2508,7 @@ typedef enum {
>>     VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN = (1 << 2),
>>     VIR_DOMAIN_DEF_FEATURE_NAME_SLASH = (1 << 3),
>>     VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4),
>> +    VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5),
> 
> Can you elaborate on why is this not the default and whydo we need to
> have a
> flag for it?  If it's just so that individual drivers can decide this,
> adding
> this to the commit message is enough and ACK with this and the above fixed.

I've described the reasons in cover letter. In short, rather than
enabling this for all drivers, we can take the safe approach and enable
it just for the drivers we know are safe. For now. Also, not every
driver we have has notion of aliases. But if it turns out that we have
enabled it for all the drivers, this flag can be dropped. Until then,
I'd like to be rather safe than sorry.

Due to Pavel's findings (Thanks! Nice catch), I will send v2 (enhanced
with news.xml entry and tests). Meanwhile, I'm pushing 02-06

Michal