7:49 a.m.
This patch prints file name of currently parsed document
if an parse error occurs. While parsing XML documments from
string, user may specify NULL as URL and the original error message
(not containing filename information) is printed.
Fixes BZ #726771
---
src/util/xml.c | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/util/xml.c b/src/util/xml.c
index 05317ea..0e93d48 100644
--- a/src/util/xml.c
+++ b/src/util/xml.c
@@ -647,10 +647,18 @@ catchXMLError(void *ctx, const char *msg ATTRIBUTE_UNUSED, ...)
if (virGetLastError() == NULL &&
ctxt->lastError.level == XML_ERR_FATAL &&
ctxt->lastError.message != NULL) {
- virGenericReportError(domcode, VIR_ERR_XML_DETAIL,
- _("at line %d: %s"),
- ctxt->lastError.line,
- ctxt->lastError.message);
+ if (ctxt->lastError.file) {
+ virGenericReportError(domcode, VIR_ERR_XML_DETAIL,
+ _("in file \'%s\'at line %d:
%s"),
+ ctxt->lastError.file,
+ ctxt->lastError.line,
+ ctxt->lastError.message);
+ } else {
+ virGenericReportError(domcode, VIR_ERR_XML_DETAIL,
+ _("at line %d: %s"),
+ ctxt->lastError.line,
+ ctxt->lastError.message);
+ }
}
}
}
--
1.7.3.4
7:49 a.m.
New subject: [libvirt] [PATCH 2/2] xml: Surpress printing "domain.xml" for parse errors on XML strings
Patch a53ab1094e93f1b6d93ad9be63d8ccc5fd19a2a9 introduces printing
file name on XML errors. This corrects the URL string to be NULL and
therefrore to print an error message not containing bogus filename
"domain.xml".
---
src/conf/domain_conf.c | 2 +-
src/security/virt-aa-helper.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ce1f3c5..35dc5fe 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7048,7 +7048,7 @@ virDomainDefParse(const char *xmlStr,
xmlDocPtr xml;
virDomainDefPtr def = NULL;
- if ((xml = virXMLParse(filename, xmlStr, "domain.xml"))) {
+ if ((xml = virXMLParse(filename, xmlStr, NULL))) {
def = virDomainDefParseNode(caps, xml, xmlDocGetRootElement(xml),
expectedVirtTypes, flags);
xmlFreeDoc(xml);
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 1e2feae..4781c7a 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -642,7 +642,7 @@ caps_mockup(vahControl * ctl, const char *xmlStr)
xmlXPathContextPtr ctxt = NULL;
xmlNodePtr root;
- if (!(xml = virXMLParseString(xmlStr, "domain.xml"))) {
+ if (!(xml = virXMLParseString(xmlStr, NULL))) {
goto cleanup;
}
--
1.7.3.4
9:53 a.m.
New subject: [libvirt] [PATCH 2/2] xml: Surpress printing "domain.xml" for parse errors on XML strings
On 08/16/2011 06:49 AM, Peter Krempa wrote:
Patch a53ab1094e93f1b6d93ad9be63d8ccc5fd19a2a9 introduces printing
file name on XML errors. This corrects the URL string to be NULL and
therefrore to print an error message not containing bogus filename
s/therefrore/therefore/
"domain.xml".
---
src/conf/domain_conf.c | 2 +-
src/security/virt-aa-helper.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
+++ b/src/conf/domain_conf.c
@@ -7048,7 +7048,7 @@ virDomainDefParse(const char *xmlStr,
xmlDocPtr xml;
virDomainDefPtr def = NULL;
- if ((xml = virXMLParse(filename, xmlStr, "domain.xml"))) {
+ if ((xml = virXMLParse(filename, xmlStr, NULL))) {
I'm not sure I follow the difference between filename and url arguments.
In virXMLParseHelper, this is resulting in the difference between
xmlCtxtReadFile(pctxt, filename, NULL, flags) and xmlCtxtReadDoc(pctxt,
string, url, NULL, flags). If I'm understanding correctly, that means
that 'url' is treated as the filename owning the string being parsed
(that is, if you had manually read the file contents yourself and pass
the contents as a string, instead of caling xmlCtxtReadFile directly,
then you would use the filename of xmlCtxtReadFile as the url of
xmlCtxtReadDoc).
I guess passing NULL as the url is okay - it just means that any errors
reported by the parser are no longer tied to a specific filename (which
is okay, because we are intercepting the error reporting, and reporting
it on behalf of just the string line number anyway).
I wonder if this patch is complete, or if there are more instances where
we are passing a url to xml parse functions even though the string was
generated rather than being tied to a real file. tools/virsh.c in
particular may need some cleanups related to this, although I don't see
it using virXMLParse.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:16 a.m.
New subject: [libvirt] [PATCH 2/2] xml: Surpress printing "domain.xml" for parse errors on XML strings
On 08/16/2011 04:53 PM, Eric Blake wrote:
On 08/16/2011 06:49 AM, Peter Krempa wrote:
> Patch a53ab1094e93f1b6d93ad9be63d8ccc5fd19a2a9 introduces printing
> file name on XML errors. This corrects the URL string to be NULL and
> therefrore to print an error message not containing bogus filename
s/therefrore/therefore/
> "domain.xml".
> ---
> src/conf/domain_conf.c | 2 +-
> src/security/virt-aa-helper.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> +++ b/src/conf/domain_conf.c
> @@ -7048,7 +7048,7 @@ virDomainDefParse(const char *xmlStr,
> xmlDocPtr xml;
> virDomainDefPtr def = NULL;
>
> - if ((xml = virXMLParse(filename, xmlStr, "domain.xml"))) {
> + if ((xml = virXMLParse(filename, xmlStr, NULL))) {
I'm not sure I follow the difference between filename and url
arguments. In virXMLParseHelper, this is resulting in the difference
between xmlCtxtReadFile(pctxt, filename, NULL, flags) and
xmlCtxtReadDoc(pctxt, string, url, NULL, flags). If I'm understanding
correctly, that means that 'url' is treated as the filename owning the
string being parsed (that is, if you had manually read the file
contents yourself and pass the contents as a string, instead of caling
xmlCtxtReadFile directly, then you would use the filename of
xmlCtxtReadFile as the url of xmlCtxtReadDoc).
Yes, URL is used as a substitute for
filename if you're parsing a XML
doc in a string. Value of URL is copied into the parser context if
filename is not specified, but there's a NULL check.
I guess passing NULL as the url is okay - it just means that any
errors reported by the parser are no longer tied to a specific
filename (which is okay, because we are intercepting the error
reporting, and reporting it on behalf of just the string line number
anyway).
Internally in libxml2 there's a check for NULL dereference on URL and
AFAIK the only place the value is used again is in error reporting. The
error reporting in libvirt is done by a custom catcher function, so I
think it's a good way to differentiate these error messages.
I wonder if this patch is complete, or if there are more instances
where we are passing a url to xml parse functions even though the
string was generated rather than being tied to a real file.
tools/virsh.c in particular may need some cleanups related to this,
although I don't see it using virXMLParse.
virsh uses the string "domain.xml" mostly for parsing libvirt's
output.
I replaced all instances while calling virXMLParse with a value of NULL
(there are exactly 2 of them).
I think, there should remain the posibility to specify the url string
for future code, but in these two instances it could mislead the user
into looking for files that don't exist.
Peter
(i messed up v2, so I'll have to post it again (with the typo in the
commit message fixed ))
9:37 a.m.
On 08/16/2011 06:49 AM, Peter Krempa wrote:
This patch prints file name of currently parsed document
if an parse error occurs. While parsing XML documments from
string, user may specify NULL as URL and the original error message
(not containing filename information) is printed.
Fixes BZ #726771
---
src/util/xml.c | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/util/xml.c b/src/util/xml.c
index 05317ea..0e93d48 100644
--- a/src/util/xml.c
+++ b/src/util/xml.c
@@ -647,10 +647,18 @@ catchXMLError(void *ctx, const char *msg ATTRIBUTE_UNUSED, ...)
if (virGetLastError() == NULL&&
ctxt->lastError.level == XML_ERR_FATAL&&
ctxt->lastError.message != NULL) {
- virGenericReportError(domcode, VIR_ERR_XML_DETAIL,
- _("at line %d: %s"),
- ctxt->lastError.line,
- ctxt->lastError.message);
+ if (ctxt->lastError.file) {
+ virGenericReportError(domcode, VIR_ERR_XML_DETAIL,
+ _("in file \'%s\'at line %d:
%s"),
Missing a space, and \' is redundant inside "" (you can get away with
just '). At a higher level, when you have both a file and a line, it is
conventional to write this in a shorter form:
"%s:%d: %s", file, line, message
+ ctxt->lastError.file,
+ ctxt->lastError.line,
+ ctxt->lastError.message);
+ } else {
+ virGenericReportError(domcode, VIR_ERR_XML_DETAIL,
+ _("at line %d: %s"),
It is only here, where you don't have a file, that including the extra
text "at line %d:" is appropriate (besides the fact that it preserves
the original error spelling).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:51 a.m.
On 08/16/2011 04:37 PM, Eric Blake wrote:
On 08/16/2011 06:49 AM, Peter Krempa wrote:
> This patch prints file name of currently parsed document
> if an parse error occurs. While parsing XML documments from
> string, user may specify NULL as URL and the original error message
> (not containing filename information) is printed.
>
> Fixes BZ #726771
> ---
> src/util/xml.c | 16 ++++++++++++----
> 1 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/xml.c b/src/util/xml.c
> index 05317ea..0e93d48 100644
> --- a/src/util/xml.c
> +++ b/src/util/xml.c
> @@ -647,10 +647,18 @@ catchXMLError(void *ctx, const char *msg
> ATTRIBUTE_UNUSED, ...)
> if (virGetLastError() == NULL&&
> ctxt->lastError.level == XML_ERR_FATAL&&
> ctxt->lastError.message != NULL) {
> - virGenericReportError(domcode, VIR_ERR_XML_DETAIL,
> - _("at line %d: %s"),
> - ctxt->lastError.line,
> - ctxt->lastError.message);
> + if (ctxt->lastError.file) {
> + virGenericReportError(domcode, VIR_ERR_XML_DETAIL,
> + _("in file \'%s\'at line %d:
> %s"),
Missing a space, and \' is redundant inside "" (you can get away with
just '). At a higher level, when you have both a file and a line, it
is conventional to write this in a shorter form:
"%s:%d: %s", file, line, message
Yes, that definitely looks better. I
somehow tried to stick with the
previous message and didn't think much about it.
> + ctxt->lastError.file,
> + ctxt->lastError.line,
> + ctxt->lastError.message);
> + } else {
> + virGenericReportError(domcode, VIR_ERR_XML_DETAIL,
> + _("at line %d: %s"),
It is only here, where you don't have a file, that including the extra
text "at line %d:" is appropriate (besides the fact that it preserves
the original error spelling).
Message fixed in v2.
Peter
4850
days inactive
4850
days old
5 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa