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

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(a)redhat.com&gt; > --- > 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.