[libvirt] [PATCH 1/2] util: Add function to check if string contains some chars

[Michal Privoznik]

On 19.10.2016 03:55, Sławek Kapłoński wrote:
> Tue, 18 Oct 2016, Michal Privoznik wrote:
>> > On 14.10.2016 04:53, Sławek Kapłoński wrote:
>>> > > This new function can be used to check if e.g. name of XML node
>>> > > don't contains forbidden chars like "/" or new-line.
>>> > > ---
>>> > >  src/conf/network_conf.c  | 2 +-
>>> > >  src/libvirt_private.syms | 1 +
>>> > >  src/util/virstring.c     | 9 +++++++++
>>> > >  src/util/virstring.h     | 1 +
>>> > >  4 files changed, 12 insertions(+), 1 deletion(-)
>>> > > 
>>> > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>>> > > index aa39776..949d138 100644
>>> > > --- a/src/conf/network_conf.c
>>> > > +++ b/src/conf/network_conf.c
>>> > > @@ -2117,7 +2117,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>>> > >          goto error;
>>> > >      }
>>> > >  
>>> > > -    if (strchr(def->name, '/')) {
>>> > > +    if (virStringHasChars(def->name, "/")) {
>>> > >          virReportError(VIR_ERR_XML_ERROR,
>>> > >                         _("name %s cannot contain '/'"), def->name);
>> > 
>> > Usually, in one patch we introduce a function and then in a subsequent
>> > one we switch the rest of the code into using it. But okay, I can live
>> > with this too.
>> > 
>>> > >          goto error;
>>> > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> > > index 55b6a24..6f41234 100644
>>> > > --- a/src/libvirt_private.syms
>>> > > +++ b/src/libvirt_private.syms
>>> > > @@ -2435,6 +2435,7 @@ virStringEncodeBase64;
>>> > >  virStringFreeList;
>>> > >  virStringFreeListCount;
>>> > >  virStringGetFirstWithPrefix;
>>> > > +virStringHasChars;
>>> > >  virStringHasControlChars;
>>> > >  virStringIsEmpty;
>>> > >  virStringIsPrintable;
>>> > > diff --git a/src/util/virstring.c b/src/util/virstring.c
>>> > > index 4a70620..7799d87 100644
>>> > > --- a/src/util/virstring.c
>>> > > +++ b/src/util/virstring.c
>>> > > @@ -975,6 +975,15 @@ virStringStripIPv6Brackets(char *str)
>>> > >  }
>>> > >  
>>> > >  
>>> > > +bool
>>> > > +virStringHasChars(const char *str, const char *chars)
>>> > > +{
>>> > > +    if (strpbrk(str, chars))
>>> > > +        return true;
>>> > > +    return false;
>>> > > +}
>> > 
>> > This, however, makes not much sense. I mean, this function has no added
>> > value to pain strpbrk(). What I suggested in one of previous reviews was
>> > that this function should report an error too. In that case, it will
>> > immediately have added value and thus it will be handy to use it.
>> > Perhaps you are afraid that error message might change in some cases?
>> > That's okay, we don't make any promises about error messages.
>> > 
> I agree that in fact it don't have too much sense but I'm not sure that
> it would be good to report error here. First of all, it could be that in
> some cases it could be used to check if function have required chars so
> there is no easy way to check which result is error in fact.

Well, even if we did want to check for that, strpbrk() is no help there.
I mean, you cannot use it to check if a string contains only allowed
characters and nothing more.

> Second thing (maybe here I'm wrong) places where I wanted to use this
> function are reporting error VIR_ERR_XML_ERROR and I'm not sure it is
> good place to report such error in "string lib".

That's why I initially suggested for this function to be in virxml.c
(and thus have a slightly different name to reflect the submodule it is in).

> So maybe better solution would be just use strbprk (or strchr) in
> src/network/bridge_driver.c to check if name contains \n char and not
> introduce any new function to such check? What You think about that?

Well, then again - I don't think we should limit ourselves to network
driver really. Other drivers suffer from the same problem, don't they? I
mean, sure - we can just use strpbrk() here and copy it all over the
place to different drivers, but I think having a small function just for
that would be more convenient.
Moreover, we already have some small, one purpose functions in virxml,
for instance:  virXMLPickShellSafeComment, virXMLSaveFile,
virXMLPropString, and others.

Michal