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

[Martin Kletzander]

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

>John
>
>>          goto cleanup;
>> +    }
>>
>>      if (VIR_ALLOC(ret) < 0)
>>          goto cleanup;
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181017/811b4fd9/attachment-0001.sig>