| Message ID | 20200612003640.16248-1-gaurav1086@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | xdp_rxq_info_user: Replace malloc/memset w/calloc | expand |
On Thu, 11 Jun 2020 20:36:40 -0400 Gaurav Singh <gaurav1086@gmail.com> wrote: > Replace malloc/memset with calloc > > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info") > Signed-off-by: Gaurav Singh <gaurav1086@gmail.com> Above is the correct use of Fixes + Signed-off-by. Now you need to update/improve the description, to also mention/describe that this also solves the bug you found.
On Fri, 2020-06-12 at 08:42 +0200, Jesper Dangaard Brouer wrote: > On Thu, 11 Jun 2020 20:36:40 -0400 > Gaurav Singh <gaurav1086@gmail.com> wrote: > > > Replace malloc/memset with calloc > > > > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info") > > Signed-off-by: Gaurav Singh <gaurav1086@gmail.com> > > Above is the correct use of Fixes + Signed-off-by. > > Now you need to update/improve the description, to also > mention/describe that this also solves the bug you found. This is not a fix, it's a conversion of one correct code to a shorter one.
On Fri, 12 Jun 2020 03:14:58 -0700 Joe Perches <joe@perches.com> wrote: > On Fri, 2020-06-12 at 08:42 +0200, Jesper Dangaard Brouer wrote: > > On Thu, 11 Jun 2020 20:36:40 -0400 > > Gaurav Singh <gaurav1086@gmail.com> wrote: > > > > > Replace malloc/memset with calloc > > > > > > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info") > > > Signed-off-by: Gaurav Singh <gaurav1086@gmail.com> > > > > Above is the correct use of Fixes + Signed-off-by. > > > > Now you need to update/improve the description, to also > > mention/describe that this also solves the bug you found. > > This is not a fix, it's a conversion of one > correct code to a shorter one. Read the code again Joe. There is a bug in the code that gets removed, as it runs memset on the memory before checking if it was NULL. IHMO this proves why is it is necessary to mention in the commit message, as you didn't notice the bug in your code review.
Joe Perches <joe@perches.com> writes: > On Fri, 2020-06-12 at 08:42 +0200, Jesper Dangaard Brouer wrote: >> On Thu, 11 Jun 2020 20:36:40 -0400 >> Gaurav Singh <gaurav1086@gmail.com> wrote: >> >> > Replace malloc/memset with calloc >> > >> > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info") >> > Signed-off-by: Gaurav Singh <gaurav1086@gmail.com> >> >> Above is the correct use of Fixes + Signed-off-by. >> >> Now you need to update/improve the description, to also >> mention/describe that this also solves the bug you found. > > This is not a fix, it's a conversion of one > correct code to a shorter one. No it isn't - the original code memset()s before it checks the return from malloc(), so it's a potential NULL-pointer reference... Which the commit message should explain, obviously :) -Toke
On Fri, 2020-06-12 at 14:05 +0200, Jesper Dangaard Brouer wrote: > On Fri, 12 Jun 2020 03:14:58 -0700 > Joe Perches <joe@perches.com> wrote: > > > On Fri, 2020-06-12 at 08:42 +0200, Jesper Dangaard Brouer wrote: > > > On Thu, 11 Jun 2020 20:36:40 -0400 > > > Gaurav Singh <gaurav1086@gmail.com> wrote: > > > > > > > Replace malloc/memset with calloc > > > > > > > > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info") > > > > Signed-off-by: Gaurav Singh <gaurav1086@gmail.com> > > > > > > Above is the correct use of Fixes + Signed-off-by. > > > > > > Now you need to update/improve the description, to also > > > mention/describe that this also solves the bug you found. > > > > This is not a fix, it's a conversion of one > > correct code to a shorter one. > > Read the code again Joe. There is a bug in the code that gets removed, > as it runs memset on the memory before checking if it was NULL. > > IHMO this proves why is it is necessary to mention in the commit > message, as you didn't notice the bug in your code review. I didn't review the code at all, just the commit message, It's important to have commit messages that describe the defect being corrected too. Otherwise, a simple malloc/memset(0) vs zalloc equivalent is not actually a defect.
diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c index 4fe47502ebed..caa4e7ffcfc7 100644 --- a/samples/bpf/xdp_rxq_info_user.c +++ b/samples/bpf/xdp_rxq_info_user.c @@ -198,11 +198,8 @@ static struct datarec *alloc_record_per_cpu(void) { unsigned int nr_cpus = bpf_num_possible_cpus(); struct datarec *array; - size_t size; - size = sizeof(struct datarec) * nr_cpus; - array = malloc(size); - memset(array, 0, size); + array = calloc(nr_cpus, sizeof(struct datarec)); if (!array) { fprintf(stderr, "Mem alloc error (nr_cpus:%u)\n", nr_cpus); exit(EXIT_FAIL_MEM); @@ -214,11 +211,8 @@ static struct record *alloc_record_per_rxq(void) { unsigned int nr_rxqs = bpf_map__def(rx_queue_index_map)->max_entries; struct record *array; - size_t size; - size = sizeof(struct record) * nr_rxqs; - array = malloc(size); - memset(array, 0, size); + array = calloc(nr_rxqs, sizeof(struct record)); if (!array) { fprintf(stderr, "Mem alloc error (nr_rxqs:%u)\n", nr_rxqs); exit(EXIT_FAIL_MEM); @@ -232,8 +226,7 @@ static struct stats_record *alloc_stats_record(void) struct stats_record *rec; int i; - rec = malloc(sizeof(*rec)); - memset(rec, 0, sizeof(*rec)); + rec = calloc(1, sizeof(struct stats_record)); if (!rec) { fprintf(stderr, "Mem alloc error\n"); exit(EXIT_FAIL_MEM);
Replace malloc/memset with calloc Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info") Signed-off-by: Gaurav Singh <gaurav1086@gmail.com> --- samples/bpf/xdp_rxq_info_user.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)