> -----Original Message----- > From: John Ferlan [mailto:jferlan@xxxxxxxxxx] > Sent: Wednesday, August 12, 2015 12:01 AM > To: Moshe Levi; Laine Stump; libvir-list@xxxxxxxxxx > Subject: Re: [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@xxxxxxxxx] On Behalf > >> Of Laine Stump > >> Sent: Tuesday, August 11, 2015 9:27 AM > >> To: libvir-list@xxxxxxxxxx > >> Cc: Moshe Levi > >> Subject: Re: [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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list