Re: [libvirt] [PATCH 1/2] util: Add function to check if string contains some chars
Hello,
--
Best regards / Pozdrawiam
Sławek Kapłoński
sla...@kaplonski.pl
On Wed, 19 Oct 2016, Michal Privoznik wrote:
> 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.
Ok, that makes sense. I will add this function to virxml.c then and will
rename it. Then it should be fine to report error message also in such
function. Once again, thx for help with that.
>
> Michal
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: Add function to check if string contains some chars
Hello,
Thx for review. Please read my answear inline.
--
Best regards / Pozdrawiam
Sławek Kapłoński
sla...@kaplonski.pl
On 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.
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".
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?
> > +
> > +
> > static const char control_chars[] =
> > "\x01\x02\x03\x04\x05\x06\x07"
> > "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F"
> > diff --git a/src/util/virstring.h b/src/util/virstring.h
> > index 8854d5f..7f2c96d 100644
> > --- a/src/util/virstring.h
> > +++ b/src/util/virstring.h
> > @@ -272,6 +272,7 @@ char *virStringReplace(const char *haystack,
> > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> >
> > void virStringStripIPv6Brackets(char *str);
> > +bool virStringHasChars(const char *str, const char *chars);
> > bool virStringHasControlChars(const char *str);
> > void virStringStripControlChars(char *str);
> >
> >
>
> Michal
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: Add function to check if string contains some chars
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.
> +
> +
> static const char control_chars[] =
> "\x01\x02\x03\x04\x05\x06\x07"
> "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F"
> diff --git a/src/util/virstring.h b/src/util/virstring.h
> index 8854d5f..7f2c96d 100644
> --- a/src/util/virstring.h
> +++ b/src/util/virstring.h
> @@ -272,6 +272,7 @@ char *virStringReplace(const char *haystack,
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>
> void virStringStripIPv6Brackets(char *str);
> +bool virStringHasChars(const char *str, const char *chars);
> bool virStringHasControlChars(const char *str);
> void virStringStripControlChars(char *str);
>
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: Add function to check if string contains some chars
On 10/10/2016 04:15 PM, Sławek Kapłoński wrote:
> This new function can be used to check if e.g. name of XML node don't
s/don't/doesn't/
> contains forbidden chars like "/" or new-line.
s/new-line/'\n'
> ---
> src/util/virxml.c | 18 ++
> src/util/virxml.h | 4
> 2 files changed, 22 insertions(+)
>
Couple of other nits...
There are a few other string manipulation API's in src/util/virstring.c
whether these changes belong there is a matter of interpretation.
Certainly there's quite a few places where you might also get some ideas
especially with respect to handling these types of things from other
parts of libvirt...
If you went with a common virstring function, it could check check and
return the 0 or -1 allowing/forcing the caller to provide the error
message...
> diff --git a/src/util/virxml.c b/src/util/virxml.c
> index 03bd784..450487e 100644
> --- a/src/util/virxml.c
> +++ b/src/util/virxml.c
> @@ -890,6 +890,24 @@ virXMLChildElementCount(xmlNodePtr node)
> return ret;
> }
>
We've been using 2 blank lines between functions lately.
> +/**
> + * virXMLCheckString: checks if string contains at least one of
> + * forbidden characters
More recent comments would be:
* virXMLCheckString:
* @nodename:
* @str:
* @forbiddenChars:
*
*
*
> + *
> + * Returns: 0 if string don't contains any of given characters, -1 otherwise
s/don't/doesn't
> + */
> +int virXMLCheckString(const char *nodeName, char *str,
> + const char *forbiddenChars)
We've been using:
int
virXMLCheckString(const char *nodeName,
char *str,
const char *forbiddenChars)
I would think 'str' is const too since you're not changing it.
> +{
> +char *c;
> +c = strpbrk(str, forbiddenChars);
> +if (c) {
if ((c = strpbrk(str, forbiddenChars))) {
> +virReportError(VIR_ERR_XML_DETAIL,
> +_("invalid char in %s: %c"), nodeName, *c);
The _( needs to be lined up below the VIR, e.g.
virReportError(VIR_ERR_XML_DETAIL,
_("invalid char in ..."), ...);
[the ... args could be on the second line]
Now if the *c is '\n' (or any other non-printable), then printing won't
do much good without some form of escaping. Alternatively the hex value
could be printed...
You may want to look at {virStringHas|virStringStrip}ControlChars too
for some more thoughts.
> +return -1;
> +}
> +return 0;
> +}
>
> /**
> * virXMLNodeToString: convert an XML node ptr to an XML string
> diff --git a/src/util/virxml.h b/src/util/virxml.h
> index 7a0a1da..676bf5e 100644
> --- a/src/util/virxml.h
> +++ b/src/util/virxml.h
> @@ -75,6 +75,10 @@ char * virXMLPropString(xmlNodePtr node,
> const char *name);
> long virXMLChildElementCount(xmlNodePtr node);
>
> +intvirXMLCheckString(const char *nodeName,
> + char *str,
> + const char *forbiddenChars);
I see you're just following existing practices in the module that's good.
John
> +
> /* Internal function; prefer the macros below. */
> xmlDocPtr virXMLParseHelper(int domcode,
> const char *filename,
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: Add function to check if string contains some chars
On 11.10.2016 04:15, 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/util/virxml.c | 18 ++
> src/util/virxml.h | 4
> 2 files changed, 22 insertions(+)
>
> diff --git a/src/util/virxml.c b/src/util/virxml.c
> index 03bd784..450487e 100644
> --- a/src/util/virxml.c
> +++ b/src/util/virxml.c
> @@ -890,6 +890,24 @@ virXMLChildElementCount(xmlNodePtr node)
> return ret;
> }
>
> +/**
> + * virXMLCheckString: checks if string contains at least one of
> + * forbidden characters
> + *
> + * Returns: 0 if string don't contains any of given characters, -1 otherwise
> + */
> +int virXMLCheckString(const char *nodeName, char *str,
> + const char *forbiddenChars)
> +{
> +char *c;
> +c = strpbrk(str, forbiddenChars);
> +if (c) {
> +virReportError(VIR_ERR_XML_DETAIL,
> +_("invalid char in %s: %c"), nodeName, *c);
> +return -1;
> +}
> +return 0;
> +}
>
> /**
> * virXMLNodeToString: convert an XML node ptr to an XML string
> diff --git a/src/util/virxml.h b/src/util/virxml.h
> index 7a0a1da..676bf5e 100644
> --- a/src/util/virxml.h
> +++ b/src/util/virxml.h
> @@ -75,6 +75,10 @@ char * virXMLPropString(xmlNodePtr node,
> const char *name);
> long virXMLChildElementCount(xmlNodePtr node);
>
> +intvirXMLCheckString(const char *nodeName,
> + char *str,
> + const char *forbiddenChars);
> +
> /* Internal function; prefer the macros below. */
> xmlDocPtr virXMLParseHelper(int domcode,
> const char *filename,
>
Looking good, however you forgot to add the symbol to
src/libvirt_private.syms. Without it, other parts of the code might not
be able to use this function if linker decides to optimize the symbol out.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
