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

Han Han posted 1 patch 5 years, 5 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20181014142642.12431-1-hhan@redhat.com
src/conf/domain_conf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[libvirt] [PATCH] conf: More clear error msg for incomplete coalesce xml
Posted by Han Han 5 years, 5 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1535930

Report more clear err msg instead of unknown error when coalesce
settings is incomplete.

Signed-off-by: Han Han <hhan@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",
+                       _("incomplete coalesce settings in interface xml"));
         goto cleanup;
+    }
 
     if (VIR_ALLOC(ret) < 0)
         goto cleanup;
-- 
2.19.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: More clear error msg for incomplete coalesce xml
Posted by John Ferlan 5 years, 5 months ago

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.
> 
> Signed-off-by: Han Han <hhan@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).

John

>          goto cleanup;
> +    }
>  
>      if (VIR_ALLOC(ret) < 0)
>          goto cleanup;
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: More clear error msg for incomplete coalesce xml
Posted by Martin Kletzander 5 years, 5 months ago
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.

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@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).

>John
>
>>          goto cleanup;
>> +    }
>>
>>      if (VIR_ALLOC(ret) < 0)
>>          goto cleanup;
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: More clear error msg for incomplete coalesce xml
Posted by Han Han 5 years, 5 months ago
On Wed, Oct 17, 2018 at 4:46 PM Martin Kletzander <mkletzan@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@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@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: More clear error msg for incomplete coalesce xml
Posted by Martin Kletzander 5 years, 5 months ago
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@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?

>> 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@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@redhat.com
>Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: More clear error msg for incomplete coalesce xml
Posted by Han Han 5 years, 5 months ago
On Thu, Oct 18, 2018 at 5:28 PM Martin Kletzander <mkletzan@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@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@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@redhat.com
> >Phone: +861065339333
>


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

Email: hhan@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list