On Wed, Oct 17, 2018 at 4:46 PM Martin Kletzander <mklet...@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 <h...@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: h...@redhat.com Phone: +861065339333
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list