3:15 p.m.
*** BLURB HERE ***
Sławek Kapłoński (2):
util: Add function to check if string contains some chars
Forbid new-line char in name of networks
src/conf/network_conf.c | 5 +----
src/util/virxml.c | 18 ++++++++++++++++++
src/util/virxml.h | 4 ++++
3 files changed, 23 insertions(+), 4 deletions(-)
--
2.10.0
3:15 p.m.
New subject: [libvirt] [PATCH 1/2] util: Add function to check if string contains some chars
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);
+int virXMLCheckString(const char *nodeName,
+ char *str,
+ const char *forbiddenChars);
+
/* Internal function; prefer the macros below. */
xmlDocPtr virXMLParseHelper(int domcode,
const char *filename,
--
2.10.0
4:46 a.m.
New subject: [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);
+int virXMLCheckString(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
5:18 p.m.
New subject: [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: <description>
* @str: <description>
* @forbiddenChars: <description>
*
* <Function description>
*
+ *
+ * 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);
+int virXMLCheckString(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,
3:15 p.m.
New subject: [libvirt] [PATCH 2/2] Forbid new-line char in name of networks
New line character in name of network is now forbidden because it
mess virsh output and can be confusing for users.
Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064
---
src/conf/network_conf.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index aa39776..a7f802b 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2117,11 +2117,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
goto error;
}
- if (strchr(def->name, '/')) {
- virReportError(VIR_ERR_XML_ERROR,
- _("name %s cannot contain '/'"), def->name);
+ if (virXMLCheckString("name", def->name, "/\n"))
goto error;
- }
/* Extract network uuid */
tmp = virXPathString("string(./uuid[1])", ctxt);
--
2.10.0
4:46 a.m.
New subject: [libvirt] [PATCH 2/2] Forbid new-line char in name of networks
On 11.10.2016 04:15, Sławek Kapłoński wrote:
New line character in name of network is now forbidden because it
mess virsh output and can be confusing for users.
Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064
---
src/conf/network_conf.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index aa39776..a7f802b 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2117,11 +2117,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
goto error;
}
- if (strchr(def->name, '/')) {
- virReportError(VIR_ERR_XML_ERROR,
- _("name %s cannot contain '/'"),
def->name);
+ if (virXMLCheckString("name", def->name, "/\n"))
goto error;
- }
/* Extract network uuid */
tmp = virXPathString("string(./uuid[1])", ctxt);
Almost. As Peter pointed out, failure to parse XML (and this is causing
failure) results in libvirt not knowing anything about the object in the
XML. For instance, if somebody already has a network with '\n' in its
name, after this is applied libvirt stops reporting the network and
leaves it behind. With no chance for users to adjust their definition.
What we should do here is to:
1) still deny '/' in network name (as this was never allowed),
2) deny networks with any other forbidden characters (like '\n' for
instance) in network startup process.
Michal
4:46 a.m.
On 11.10.2016 04:15, Sławek Kapłoński wrote:
*** BLURB HERE ***
Sławek Kapłoński (2):
util: Add function to check if string contains some chars
Forbid new-line char in name of networks
src/conf/network_conf.c | 5 +----
src/util/virxml.c | 18 ++++++++++++++++++
src/util/virxml.h | 4 ++++
3 files changed, 23 insertions(+), 4 deletions(-)
Usually, we prefix new versions of the patches with 'PATCH v2 ...' for
version 2, 'PATCH v3 ...' for version 3, and so on. And on the top of
that, we describe what are the changes compared to previous version(s).
This is a good example:
https://www.redhat.com/archives/libvir-list/2016-October/msg00306.html
Michal
2963
days inactive
2965
days old
6 comments
3 participants
participants (3)
-
John Ferlan -
Michal Privoznik -
Sławek Kapłoński