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

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