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

[Han Han]

On Thu, Oct 18, 2018 at 5:28 PM Martin Kletzander <mkletzan at redhat.com>
wrote:

> 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 at redhat.com>
> >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 at redhat.com>
> >> >> ---
> >> >>  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 at redhat.com
> >Phone: +861065339333
>


-- 
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.

Email: hhan at redhat.com
Phone: +861065339333
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181029/76fcbd8b/attachment-0001.htm>