| Message ID | 20200924192758.577595-1-ivecera@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool,1/2] netlink: return -ENOMEM when calloc fails | expand |
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
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
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)
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(-)