[libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel

[John Ferlan]

On 08/11/2015 03:28 AM, Moshe Levi wrote:
> 
> 
>> -----Original Message-----
>> From: sendmail [mailto:justsendmailnothingelse at gmail.com] On Behalf Of
>> Laine Stump
>> Sent: Tuesday, August 11, 2015 9:27 AM
>> To: libvir-list at redhat.com
>> Cc: Moshe Levi
>> Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to
>> running kernel
>>
>> On 08/08/2015 05:34 AM, Moshe Levi wrote:
>>> This patch add virNetDevGetGFeaturesSize to get the supported gfeature
>>> size from the kernel
>>> ---
>>
>> This is interesting/possibly useful, but it doesn't fix the crash that users are
>> experiencing. Here is a patch that should fix the crash:
>>
>> https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
>>
>> I would rather have that patch pushed before this one (which will mean
>> rebasing and resolving some merge conflicts).
> 
> Ok I will rebase once you patch is merged. 

Laine's patch is now pushed - I assume at least parts of this will be
necessary since there are reports of different GFEATURE_SIZE values...

...<snip>...

>> virNetDevSendEthtoolIoctl() logs an error message, but it looks like you want
>> for an error to be swallowed here (just returning size = 0). If that's the case
>> then virNetDevSendEthtoolIoctl() needs to be reworked to not log errors,
>> then every caller to it needs to log the error.
> This was comment by John Ferlan he preferred the method will return size 0 if not supported 
> or error. My previous patch which I send to him directly and not the ML return -1 on failure. 
> But thinking about this again I don't want to swallow if error occur. 
> What do you think? 
> 

I think my non ML response to you was more to the effect of what purpose
is returning "size = 0" and it certainly wasn't perfectly clear what
size = 0 to the caller meant...  Taken out of context regarding the
changes you sent me, my comment was:

"Then virNetDevGetGFeaturesSize can return -1 or a size, but it's not
clear whether "size == 0" could be true. The code only checks if size ==
-1 to fail and spits out another VIR_DEBUG message (one would already be
spit out on ioctl() failures, so that's duplicitous). So the question is
- is it possible to return size == 0 and if so, I assume that wouldn't
be good."

and to be fair I was reading that code after driving 13 hours while
moving ;-)

I do agree with some of the changes Laine posted in his first pass at
fixing some inconsistencies in the code in one large patch (which were
requested to be split up). Some issues were not a result of your
patches, but previous patches which were more or less reused. In the
long run if more patches are added to clean things up - that'll be good.
 We move forward and learn from our mistakes.


John