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

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, August 12, 2015 12:01 AM To: Moshe Levi; Laine Stump; libvir-list(a)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@gmail.com] On Behalf >> Of Laine Stump >> Sent: Tuesday, August 11, 2015 9:27 AM >> To: libvir-list(a)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