[libvirt] [PATCHv2] util: fix virNetDevSendEthtoolIoctl() and its callers

[Ján Tomko]

[reducing the cc-list]

On Tue, Aug 11, 2015 at 02:47:09AM -0400, Laine Stump wrote:
> This started out as a fix for a crash reported in IRC and on libvir-list:
> 
>  https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
> 
> but as I examined the existing code I found so many small nits to pick
> in surrounding functions that I decided to fix them all in this patch.
> 
> The most important fix here is that virNetDevGetFeatures was creating
> a struct ethtool_gfeatures object as a local on the stack and,
> although the struct is defined with 0 elements in its features array,
> we were telling the ethtool ioctl that we had made space for 2
> elements. This led to a crash, as outlined in the email above. The fix
> for this is to allocate the memory for the ethtool_gfeatures object
> using VIR_ALLOC_VAR, adding on GFEATURES_SIZE elements of type
> ethtool_get_features_block
> 
> Beyond that crash fixer, the following fixups were made to the
> hierarchy of functions between virNetDevGetFeatures() and
> virNetDevSendEthtoolIoctl():

This looks like an enumeration of things that should've been separate :)

> 
> * macros used to examine the gfeatures bits were renamed from
>   FEATURE_* to GFEATURE_*
> 
> virNetDevSentEthtoolIoctl():
> 
> * no need to initialize sock to -1, since it is always set at the top
>   of the function.

No functional changes here.

> 
> * remove VIR_DEBUG log (and subsequent *skipping* of error log!) when
>   errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were
>   ever encountered, this would have been *very* problematic, as it
>   would have led to one of those situations where virsh reports "an
>   error was encountered but the cause is unknown" (or whatever the
>   message is when we have an error but no log message).
> 

This does not seem problematic, since we do not treat -1 as a failure.

> * always call VIR_FORCE_CLOSE(sock) since we know that sock is either
>   a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip).
> 

No fucntional change there either.

> virNetDevFeatureAvailable()
> 
> * simplify it - no need for ret.
> 
> * follow libvirt convention of checking for "bobLobLaw(lawblog) < 0"
>   instead of "!bobLobLaw(lawblog)".
> 
> virNetDevGFeatureAvailable()
> 
> * eliminate this function, as it was ill-conceived (it really was only
>   checking for one gfeature (TX_UDP_TNL), not *any* gfeature.
> 
> virNetDevGetFeatures()
> 
> * move all data tables (struct elem) out of the function so that they
>   will be statics instead of taking up space on the stack.
> 
> * remove pointless/incorrect initialization of i = -1.
> 
> * remove unnecessary static initialization of struct ethtool_value cmd
> 
> * g_cmd is now a pointer instead of automatic
> 
> * use libvirt convention of checking return from
>   virNetDevFeatureAvailable() < 0, instead of
>   "!virNetDevFeatureAvailable()", and immediately return to caller
>   with an error when virNetDevFeatureAvailable() returns an error
>   (previously it was ignored).
> 

This is the change that makes the lack of error reporting problematic.

> * remove superfluous size_t j, and just re-use i instead.
> 

Another style cleanup that could be separated.

> * runtime allocation/free of proper size object for ethtoolfeatures as
>   described above.
> 
> * directly call virNetDevSentEthtoolIoctl() instead of now defunct
>   virNetDevGFeatureAvailable().
> 
> ---
> 
> V2: I had been looking for something like VIR_ALLOC_VAR() when writing
> the patch originally, but somehow missed it, and did an ugly hack with
> VIR_ALLOC_N and a union. In this version I clean that up and use
> VIR_ALLOC_VAR() instead.
> 
> NB: This patch does *not* attempt to determine the proper number of
> elements for the gfeature array at runtime, as proposed in this patch:
> 
>   https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html
> 
> since that wasn't the cause of the crash. I'll leave it up to Moshe to
> repost that patch rebased around this one (or whatever ends up being
> pushed) if he thinks there is value to it.
> 
> Also - as I mentioned in earlier mail in response to the crash, I
> noticed when looking into the gfeatures ethtool code that it looks to
> me like TX_UDP_TNL should actually be 26 rather than 25, but I may be
> missing something. Moshe - can you either confirm or deny that? Where
> did you get the value 25 from?
>  src/util/virnetdev.c | 177 +++++++++++++++++++++++----------------------------
>  1 file changed, 79 insertions(+), 98 deletions(-)
> 
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 1e20789..05fbff5 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -91,10 +91,10 @@ VIR_LOG_INIT("util.netdev");
>  #if HAVE_DECL_ETHTOOL_GFEATURES
>  # define TX_UDP_TNL 25
>  # define GFEATURES_SIZE 2

Do we need to (re)define these? I'd expect these constants to be present
in some header file.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150811/be511508/attachment-0001.sig>