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

[Moshe Levi]

> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Wednesday, August 12, 2015 12:01 AM
> To: Moshe Levi; Laine Stump; libvir-list at redhat.com
> Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to
> running kernel
> 
> 
> 
> 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...
Ok, Do you want me to rebase my patch on top on this 
http://libvirt.org/git/?p=libvirt.git;a=commit;h=bfaaa2b681018f3705bae17c001700a03f67d7c4 
and fixing all Laine comments or to wait for the  cleanup patch you mention below?

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