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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list