[libvirt] [PATCH 2/2] xml: Surpress printing "domain.xml" for parse errors on XML strings

Peter Krempa pkrempa at redhat.com
Tue Aug 16 15:16:05 UTC 2011


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 ))




More information about the libvir-list mailing list