Re: [PATCH] nvmet-fc: use zero-sized array and struct_size() in kzalloc()
On 3/12/19 2:33 PM, Christoph Hellwig wrote: > Thanks, > > applied to nvme-5.2. > Great. Thanks, Christoph. -- Gustavo
Re: [PATCH] nvmet-fc: use zero-sized array and struct_size() in kzalloc()
Thanks, applied to nvme-5.2.
Re: [PATCH] nvmet-fc: use zero-sized array and struct_size() in kzalloc()
On 2/23/2019 10:51 AM, Gustavo A. R. Silva wrote:
Update the code to use a zero-sized array instead of a pointer in
structure nvmet_fc_tgt_queue and use struct_size() in kzalloc().
Notice that one of the more common cases of allocation size calculations
is finding the size of a structure that has a zero-sized array at the end,
along with memory for some number of elements for that array. For example:
struct foo {
int stuff;
struct boo entry[];
};
instance = kzalloc(sizeof(struct foo) + sizeof(struct boo) * count, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can now
use the new struct_size() helper:
instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
drivers/nvme/target/fc.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 1e9654f04c60..23baec38f97e 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -128,12 +128,12 @@ struct nvmet_fc_tgt_queue {
struct nvmet_cq nvme_cq;
struct nvmet_sq nvme_sq;
struct nvmet_fc_tgt_assoc *assoc;
- struct nvmet_fc_fcp_iod *fod; /* array of fcp_iods */
struct list_headfod_list;
struct list_headpending_cmd_list;
struct list_headavail_defer_list;
struct workqueue_struct *work_q;
struct kref ref;
+ struct nvmet_fc_fcp_iod fod[]; /* array of fcp_iods */
} __aligned(sizeof(unsigned long long));
struct nvmet_fc_tgt_assoc {
@@ -588,9 +588,7 @@ nvmet_fc_alloc_target_queue(struct nvmet_fc_tgt_assoc
*assoc,
if (qid > NVMET_NR_QUEUES)
return NULL;
- queue = kzalloc((sizeof(*queue) +
- (sizeof(struct nvmet_fc_fcp_iod) * sqsize)),
- GFP_KERNEL);
+ queue = kzalloc(struct_size(queue, fod, sqsize), GFP_KERNEL);
if (!queue)
return NULL;
@@ -603,7 +601,6 @@ nvmet_fc_alloc_target_queue(struct nvmet_fc_tgt_assoc *assoc,
if (!queue->work_q)
goto out_a_put;
- queue->fod = (struct nvmet_fc_fcp_iod *)[1];
queue->qid = qid;
queue->sqsize = sqsize;
queue->assoc = assoc;
Reviewed-by: James Smart
I guess this is a better style.
-- james
Re: [PATCH] nvmet-fc: use zero-sized array and struct_size() in kzalloc()
James, can you take a look at this one?
On Sat, Feb 23, 2019 at 12:51:08PM -0600, Gustavo A. R. Silva wrote:
> Update the code to use a zero-sized array instead of a pointer in
> structure nvmet_fc_tgt_queue and use struct_size() in kzalloc().
>
> Notice that one of the more common cases of allocation size calculations
> is finding the size of a structure that has a zero-sized array at the end,
> along with memory for some number of elements for that array. For example:
>
> struct foo {
> int stuff;
> struct boo entry[];
> };
>
> instance = kzalloc(sizeof(struct foo) + sizeof(struct boo) * count,
> GFP_KERNEL);
>
> Instead of leaving these open-coded and prone to type mistakes, we can now
> use the new struct_size() helper:
>
> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>
> This code was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/nvme/target/fc.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index 1e9654f04c60..23baec38f97e 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -128,12 +128,12 @@ struct nvmet_fc_tgt_queue {
> struct nvmet_cq nvme_cq;
> struct nvmet_sq nvme_sq;
> struct nvmet_fc_tgt_assoc *assoc;
> - struct nvmet_fc_fcp_iod *fod; /* array of fcp_iods */
> struct list_headfod_list;
> struct list_headpending_cmd_list;
> struct list_headavail_defer_list;
> struct workqueue_struct *work_q;
> struct kref ref;
> + struct nvmet_fc_fcp_iod fod[]; /* array of fcp_iods */
> } __aligned(sizeof(unsigned long long));
>
> struct nvmet_fc_tgt_assoc {
> @@ -588,9 +588,7 @@ nvmet_fc_alloc_target_queue(struct nvmet_fc_tgt_assoc
> *assoc,
> if (qid > NVMET_NR_QUEUES)
> return NULL;
>
> - queue = kzalloc((sizeof(*queue) +
> - (sizeof(struct nvmet_fc_fcp_iod) * sqsize)),
> - GFP_KERNEL);
> + queue = kzalloc(struct_size(queue, fod, sqsize), GFP_KERNEL);
> if (!queue)
> return NULL;
>
> @@ -603,7 +601,6 @@ nvmet_fc_alloc_target_queue(struct nvmet_fc_tgt_assoc
> *assoc,
> if (!queue->work_q)
> goto out_a_put;
>
> - queue->fod = (struct nvmet_fc_fcp_iod *)[1];
> queue->qid = qid;
> queue->sqsize = sqsize;
> queue->assoc = assoc;
> --
> 2.20.1
---end quoted text---
Re: [PATCH] nvmet-fc: use zero-sized array and struct_size() in kzalloc()
On 2/23/19 2:28 PM, Gustavo A. R. Silva wrote:
> Hey Joe,
>
> On 2/23/19 2:05 PM, Joe Perches wrote:
>> On Sat, 2019-02-23 at 12:51 -0600, Gustavo A. R. Silva wrote:
>>> Update the code to use a zero-sized array instead of a pointer in
>>> structure nvmet_fc_tgt_queue and use struct_size() in kzalloc().
>> []
>>> This code was detected with the help of Coccinelle.
>>
>> Really?
>> Impressive script that found this one.
>>
>
> See my comments below.
>
>>> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
>> []
>>> @@ -128,12 +128,12 @@ struct nvmet_fc_tgt_queue {
>>> struct nvmet_cq nvme_cq;
>>> struct nvmet_sq nvme_sq;
>>> struct nvmet_fc_tgt_assoc *assoc;
>>> - struct nvmet_fc_fcp_iod *fod; /* array of fcp_iods */
>>> struct list_headfod_list;
>>> struct list_headpending_cmd_list;
>>> struct list_headavail_defer_list;
>>> struct workqueue_struct *work_q;
>>> struct kref ref;
>>> + struct nvmet_fc_fcp_iod fod[]; /* array of fcp_iods */
>>> } __aligned(sizeof(unsigned long long));
>>
>> Moving a pointer from the middle of a struct to
>> the end seems unusual for coccinelle.
>>
>>
>
> Notice that the commit log says "detected", which does not imply
> the script made the transformation by itself. :)
>
> And all the script detected was this piece of code:
>
> queue = kzalloc((sizeof(*queue) +
> (sizeof(struct nvmet_fc_fcp_iod) * sqsize)),
> GFP_KERNEL);
>
>
Which is enough to mention the tool.
Thanks
--
Gustavo
Re: [PATCH] nvmet-fc: use zero-sized array and struct_size() in kzalloc()
Hey Joe,
On 2/23/19 2:05 PM, Joe Perches wrote:
> On Sat, 2019-02-23 at 12:51 -0600, Gustavo A. R. Silva wrote:
>> Update the code to use a zero-sized array instead of a pointer in
>> structure nvmet_fc_tgt_queue and use struct_size() in kzalloc().
> []
>> This code was detected with the help of Coccinelle.
>
> Really?
> Impressive script that found this one.
>
See my comments below.
>> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> []
>> @@ -128,12 +128,12 @@ struct nvmet_fc_tgt_queue {
>> struct nvmet_cq nvme_cq;
>> struct nvmet_sq nvme_sq;
>> struct nvmet_fc_tgt_assoc *assoc;
>> -struct nvmet_fc_fcp_iod *fod; /* array of fcp_iods */
>> struct list_headfod_list;
>> struct list_headpending_cmd_list;
>> struct list_headavail_defer_list;
>> struct workqueue_struct *work_q;
>> struct kref ref;
>> +struct nvmet_fc_fcp_iod fod[]; /* array of fcp_iods */
>> } __aligned(sizeof(unsigned long long));
>
> Moving a pointer from the middle of a struct to
> the end seems unusual for coccinelle.
>
>
Notice that the commit log says "detected", which does not imply
the script made the transformation by itself. :)
And all the script detected was this piece of code:
queue = kzalloc((sizeof(*queue) +
(sizeof(struct nvmet_fc_fcp_iod) * sqsize)),
GFP_KERNEL);
Thanks
--
Gustavo
Re: [PATCH] nvmet-fc: use zero-sized array and struct_size() in kzalloc()
On Sat, 2019-02-23 at 12:51 -0600, Gustavo A. R. Silva wrote:
> Update the code to use a zero-sized array instead of a pointer in
> structure nvmet_fc_tgt_queue and use struct_size() in kzalloc().
[]
> This code was detected with the help of Coccinelle.
Really?
Impressive script that found this one.
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
[]
> @@ -128,12 +128,12 @@ struct nvmet_fc_tgt_queue {
> struct nvmet_cq nvme_cq;
> struct nvmet_sq nvme_sq;
> struct nvmet_fc_tgt_assoc *assoc;
> - struct nvmet_fc_fcp_iod *fod; /* array of fcp_iods */
> struct list_headfod_list;
> struct list_headpending_cmd_list;
> struct list_headavail_defer_list;
> struct workqueue_struct *work_q;
> struct kref ref;
> + struct nvmet_fc_fcp_iod fod[]; /* array of fcp_iods */
> } __aligned(sizeof(unsigned long long));
Moving a pointer from the middle of a struct to
the end seems unusual for coccinelle.
