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

[Han Han]

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.

> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181018/6ac3ceec/attachment-0001.htm>