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

On Thu, Oct 18, 2018 at 02:12:36PM +0800, Han Han wrote: >On Wed, Oct 17, 2018 at 4:46 PM Martin Kletzander <mkletzan(a)redhat.com&gt; >wrote: > >> 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 >an inactive >VM as https://bugzilla.redhat.com/show_bug.cgi?id=1535930#c4 . > >I am not sure it will be something wrong when update the device lively, but >we can fix >this first. > So I looked at the code and yes, there is inconsistency in the parsing. If the function does not parse anything it just returns NULL without an error set and the caller treats that as an error. What we should do is pass the dev into the function, make the function return int (0 for OK, -1 for error) and update the coalesce in the passed parameter. That way empty (incomplete) coalesce will not error out, but will just set nothing in the device. This is not the whole fix, though. What is missing is the fact that if such device is being updated, then we need to see if everything was reset to zero and reset the pointer and free the coalesce struct from memory (ideally). Although there shouldn't be a code that will fail if the struct is allocated but everything is zero. Would you mind having a look at that in v2? OK. I will try to make a whole fix in v2.

>> 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). >> >Set `rx-frames=0` is a better solution compared with report a error to >incomplete >the incomplete coalesce setting. > >> >> >John >> > >> >> goto cleanup; >> >> + } >> >> >> >> if (VIR_ALLOC(ret) < 0) >> >> goto cleanup; >> >> >> > > >-- >Best regards, >----------------------------------- >Han Han >Quality Engineer >Redhat. > >Email: hhan(a)redhat.com >Phone: +861065339333