Re: [libvirt] [PATCH] conf: More clear error msg for incomplete coalesce xml

On Tue, Oct 16, 2018 at 07:19:41PM -0400, John Ferlan wrote: > > >On 10/14/18 10:26 AM, Han Han wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1535930 >> >> Report more clear err msg instead of unknown error when coalesce >> settings is incomplete. >> Incomplete is not an error. It's request for removal of the missing setting. There is no problem if it is incomplete. Well, I think incomplete xml is the problem because I have reproduce it on

If you look at the BZ the problem is not the incomplete XML, but the fact that the code that should update the device fails somewhere without setting an error. Actually, there should not be an error, it should work. So this just works around the actual issue. Having said that maybe the problem is somewhere in the parsing part, but this is not the solution. We need to "go deeper" to find out why the updating code fails and then figure out what to fix from there, not put a sheet over the problem. >> Signed-off-by: Han Han <hhan(a)redhat.com&gt; >> --- >> src/conf/domain_conf.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 9911d56130..e755f45d3d 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -7804,8 +7804,12 @@ virDomainNetDefCoalesceParseXML(xmlNodePtr node, >> ctxt->node = node; >> >> str = virXPathString("string(./rx/frames/@max)", ctxt); >> - if (!str) >> + if (!str) { >> + virReportError(VIR_ERR_XML_DETAIL, >> + "%s", > >This can be put on the previous line > >> + _("incomplete coalesce settings in interface xml")); > >and specifically this could be is missing rx frames max attributes > >However, according to the RNG from commit 523c9960, it seems the 'rx' is >optional as is the '@max' value. Maybe Martin should provide a comment >on this series since he added it. > >Of course that would cause the whole <coalesce> to disappear on Format. >It would also cause problems because def->coalesce would have something >that's empty. > >So perhaps the best thing to do is pass the @def into here, then only if >we get beyond the initial !str comparison do we allocate and fill it in; >otherwise, we return 0 if rx/frames/@max is not there. Prepares us for >the future. > >I guess I'm not 100% clear if max frames == 0 what would happen. Maybe >Martin knows (I've CC'd him). > `rx-frames=0` turns that option off. It's like not having the parameter there at all (it also works like this with ethtool).

>John > >> goto cleanup; >> + } >> >> if (VIR_ALLOC(ret) < 0) >> goto cleanup; >>