On Thu, Oct 18, 2018 at 5:28 PM Martin Kletzander <mklet...@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 <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. > > > > 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 <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 > -- 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
