Re: [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails

[Ivan Vecera <ivecera@redhat.com>]

On Mon, 28 Sep 2020 17:44:55 +0200
Michal Kubecek <mkubecek@suse.cz> wrote:

> On Thu, Sep 24, 2020 at 09:27:57PM +0200, Ivan Vecera wrote:
> > Fixes: f2c17e107900 ("netlink: add netlink handler for gfeatures (-k)")
> > 
> > Cc: Michal Kubecek <mkubecek@suse.cz>
> > Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> > ---
> >  netlink/features.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/netlink/features.c b/netlink/features.c
> > index 3f1240437350..b2cf57eea660 100644
> > --- a/netlink/features.c
> > +++ b/netlink/features.c
> > @@ -112,16 +112,17 @@ int dump_features(const struct nlattr *const *tb,
> >  	unsigned int *feature_flags = NULL;
> >  	struct feature_results results;
> >  	unsigned int i, j;
> > -	int ret;
> > +	int ret = 0;
> >  
> >  	ret = prepare_feature_results(tb, &results);
> >  	if (ret < 0)
> >  		return -EFAULT;
> >  
> > -	ret = -ENOMEM;
> >  	feature_flags = calloc(results.count, sizeof(feature_flags[0]));
> > -	if (!feature_flags)
> > +	if (!feature_flags) {
> > +		ret = -ENOMEM;
> >  		goto out_free;
> > +	}
> >  
> >  	/* map netdev features to legacy flags */
> >  	for (i = 0; i < results.count; i++) {
> > @@ -184,7 +185,7 @@ int dump_features(const struct nlattr *const *tb,
> >  
> >  out_free:
> >  	free(feature_flags);
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  int features_reply_cb(const struct nlmsghdr *nlhdr, void *data)
> > -- 
> > 2.26.2  
> 
> The patch is correct but relying on ret staying zero through the whole
> function is rather fragile (it could break when adding more checks in
> the future) and it also isn't consistent with the way this is done in
> other functions.
> 
> AFAICS you could omit the first hunk and just add "ret = 0" above the
> out_free label.
> 
> Michal

OK, will do.

Ivan