On Thu, May 21, 2015 at 13:08:12 +0800, Luyao Huang wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1223631 > > When set a memory device in the xml, <sysinfo> in xml will be lost. > Because we forgot restore ctxt->node to the oldnode after parse memory > device, this will make the parse function after virDomainMemoryDefParseXML > cannot find a node they need when parse a full xml(virDomainDefParseXML). The commit message is really hard to read.

> Signed-off-by: Luyao Huang <lhuang(a)redhat.com&gt; > --- > src/conf/domain_conf.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index bfdc94e..7ddc1ea 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -11828,6 +11828,7 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode, > if (virDomainDeviceInfoParseXML(memdevNode, NULL, &def->info, flags) < 0) > goto error; > > + ctxt->node = save; > return def; A test case is missing. The bugzilla link contains one so I'll add it.

ACK to the code. I'll rewrite the commit message and add a test case.

Peter

On Thu, May 21, 2015 at 05:52:01PM +0800, lhuang wrote: > On 05/21/2015 03:46 PM, Peter Krempa wrote: >> On Thu, May 21, 2015 at 13:08:12 +0800, Luyao Huang wrote: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1223631 >>> >>> When set a memory device in the xml, <sysinfo> in xml will be lost. >>> Because we forgot restore ctxt->node to the oldnode after parse memory >>> device, this will make the parse function after virDomainMemoryDefParseXML >>> cannot find a node they need when parse a full xml(virDomainDefParseXML). >> The commit message is really hard to read. > Okay, I will pay more attention about this. Maybe use the exist similar > commit message as an example will be a good choice. > >>> Signed-off-by: Luyao Huang <lhuang(a)redhat.com&gt; >>> --- >>> src/conf/domain_conf.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>> index bfdc94e..7ddc1ea 100644 >>> --- a/src/conf/domain_conf.c >>> +++ b/src/conf/domain_conf.c >>> @@ -11828,6 +11828,7 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode, >>> if (virDomainDeviceInfoParseXML(memdevNode, NULL, &def->info, flags) < 0) >>> goto error; >>> + ctxt->node = save; >>> return def; >> A test case is missing. The bugzilla link contains one so I'll add it. > Oh, I see, I hesitated about if need introduce a unit test when i fix this > issue. > Seems this is a rule about which case need a new test, isn't it? :) Our rule is that any addition to the XML parser needs to have a test case added - especially true if the addition is fixing a parsing bug :-)

Regards, Daniel