Re: [PATCH] xdp_rxq_info_user: Replace malloc/memset w/calloc
On Fri, 2020-06-12 at 14:05 +0200, Jesper Dangaard Brouer wrote:
> On Fri, 12 Jun 2020 03:14:58 -0700
> Joe Perches 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 wrote:
> > >
> > > > Replace malloc/memset with calloc
> > > >
> > > > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to
> > > > xdp_rxq_info")
> > > > Signed-off-by: Gaurav Singh
> > >
> > > 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.
Re: [PATCH] xdp_rxq_info_user: Replace malloc/memset w/calloc
Joe Perches 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 wrote:
>>
>> > Replace malloc/memset with calloc
>> >
>> > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to
>> > xdp_rxq_info")
>> > Signed-off-by: Gaurav Singh
>>
>> 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
Re: [PATCH] xdp_rxq_info_user: Replace malloc/memset w/calloc
On Fri, 12 Jun 2020 03:14:58 -0700
Joe Perches 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 wrote:
> >
> > > Replace malloc/memset with calloc
> > >
> > > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to
> > > xdp_rxq_info")
> > > Signed-off-by: Gaurav Singh
> >
> > 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.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
Re: [PATCH] xdp_rxq_info_user: Replace malloc/memset w/calloc
On Fri, 2020-06-12 at 08:42 +0200, Jesper Dangaard Brouer wrote:
> On Thu, 11 Jun 2020 20:36:40 -0400
> Gaurav Singh wrote:
>
> > Replace malloc/memset with calloc
> >
> > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to
> > xdp_rxq_info")
> > Signed-off-by: Gaurav Singh
>
> 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.
Re: [PATCH] xdp_rxq_info_user: Replace malloc/memset w/calloc
On Thu, 11 Jun 2020 20:36:40 -0400
Gaurav Singh wrote:
> Replace malloc/memset with calloc
>
> Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to
> xdp_rxq_info")
> Signed-off-by: Gaurav Singh
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.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
[PATCH] xdp_rxq_info_user: Replace malloc/memset w/calloc
Replace malloc/memset with calloc
Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to
xdp_rxq_info")
Signed-off-by: Gaurav Singh
---
samples/bpf/xdp_rxq_info_user.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
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);
--
2.17.1
[PATCH] xdp_rxq_info_user: Replace malloc/memset w/calloc
Replace malloc/memset with calloc
Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to
xdp_rxq_info") Signed-off-by: Gaurav Singh
---
samples/bpf/xdp_rxq_info_user.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
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);
--
2.17.1
Re: [PATCH] xdp_rxq_info_user: Replace malloc/memset w/calloc
On Thu, 11 Jun 2020 11:02:21 -0400
Gaurav Singh wrote:
> Replace malloc/memset with calloc
Please also mention/describe that this also solves the bug you found.
As this fix a potential bug, it will be appropriate to add a "Fixes:"
line, just before "Signed-off-by" (meaning no newline between the two).
Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to
xdp_rxq_info")
> Signed-off-by: Gaurav Singh
> ---
> samples/bpf/xdp_rxq_info_user.c | 13 +++--
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> 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);
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
[PATCH] xdp_rxq_info_user: Replace malloc/memset w/calloc
Replace malloc/memset with calloc
Signed-off-by: Gaurav Singh
---
samples/bpf/xdp_rxq_info_user.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
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);
--
2.17.1
