| Message ID | 033d8595-d051-1fa8-95b1-5d2056eb5667@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 17.08.2016 20:06, schrieb SF Markus Elfring: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 17 Aug 2016 18:29:04 +0200 > > Replace the specification of data structures by pointer dereferences > to make the corresponding size determination a bit safer according to > the Linux coding style convention. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > arch/s390/kvm/guestdbg.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c > index d1f8241..b68db4b 100644 > --- a/arch/s390/kvm/guestdbg.c > +++ b/arch/s390/kvm/guestdbg.c > @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT) > return -EINVAL; > > - size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint); > + size = dbg->arch.nr_hw_bp * sizeof(*bp_data); > bp_data = kmalloc(size, GFP_KERNEL); > if (!bp_data) { > ret = -ENOMEM; > @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > } > } > > - size = nr_wp * sizeof(struct kvm_hw_wp_info_arch); > + size = nr_wp * sizeof(*wp_info); > if (size > 0) { > wp_info = kmalloc(size, GFP_KERNEL); > if (!wp_info) { > @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > goto error; > } > } > - size = nr_bp * sizeof(struct kvm_hw_bp_info_arch); > + size = nr_bp * sizeof(*bp_info); > if (size > 0) { > bp_info = kmalloc(size, GFP_KERNEL); > if (!bp_info) { IMHO the common pattern for kmalloc is bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL); i can not remember code with a check for size < 0, i guess it is here to avoid an overflow ? since kmalloc takes a size_t argument this would cause a malloc failure an can be ignored. just my 2 cents. re, wh -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 18 Aug 2016, walter harms wrote: > > > Am 17.08.2016 20:06, schrieb SF Markus Elfring: > > From: Markus Elfring <elfring@users.sourceforge.net> > > Date: Wed, 17 Aug 2016 18:29:04 +0200 > > > > Replace the specification of data structures by pointer dereferences > > to make the corresponding size determination a bit safer according to > > the Linux coding style convention. > > > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > --- > > arch/s390/kvm/guestdbg.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c > > index d1f8241..b68db4b 100644 > > --- a/arch/s390/kvm/guestdbg.c > > +++ b/arch/s390/kvm/guestdbg.c > > @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > > else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT) > > return -EINVAL; > > > > - size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint); > > + size = dbg->arch.nr_hw_bp * sizeof(*bp_data); > > bp_data = kmalloc(size, GFP_KERNEL); > > if (!bp_data) { > > ret = -ENOMEM; > > @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > > } > > } > > > > - size = nr_wp * sizeof(struct kvm_hw_wp_info_arch); > > + size = nr_wp * sizeof(*wp_info); > > if (size > 0) { > > wp_info = kmalloc(size, GFP_KERNEL); > > if (!wp_info) { > > @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > > goto error; > > } > > } > > - size = nr_bp * sizeof(struct kvm_hw_bp_info_arch); > > + size = nr_bp * sizeof(*bp_info); > > if (size > 0) { > > bp_info = kmalloc(size, GFP_KERNEL); > > if (!bp_info) { > > > IMHO the common pattern for kmalloc is > bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL); > > i can not remember code with a check for size < 0, i guess it is here > to avoid an overflow ? since kmalloc takes a size_t argument this would cause > a malloc failure an can be ignored. Shoudn't it be kcalloc? julia > > > just my 2 cents. > re, > wh > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/08/2016 11:02, Julia Lawall wrote: > > > On Thu, 18 Aug 2016, walter harms wrote: > >> >> >> Am 17.08.2016 20:06, schrieb SF Markus Elfring: >>> From: Markus Elfring <elfring@users.sourceforge.net> >>> Date: Wed, 17 Aug 2016 18:29:04 +0200 >>> >>> Replace the specification of data structures by pointer dereferences >>> to make the corresponding size determination a bit safer according to >>> the Linux coding style convention. >>> >>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >>> --- >>> arch/s390/kvm/guestdbg.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c >>> index d1f8241..b68db4b 100644 >>> --- a/arch/s390/kvm/guestdbg.c >>> +++ b/arch/s390/kvm/guestdbg.c >>> @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>> else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT) >>> return -EINVAL; >>> >>> - size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint); >>> + size = dbg->arch.nr_hw_bp * sizeof(*bp_data); >>> bp_data = kmalloc(size, GFP_KERNEL); >>> if (!bp_data) { >>> ret = -ENOMEM; >>> @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>> } >>> } >>> >>> - size = nr_wp * sizeof(struct kvm_hw_wp_info_arch); >>> + size = nr_wp * sizeof(*wp_info); >>> if (size > 0) { >>> wp_info = kmalloc(size, GFP_KERNEL); >>> if (!wp_info) { >>> @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>> goto error; >>> } >>> } >>> - size = nr_bp * sizeof(struct kvm_hw_bp_info_arch); >>> + size = nr_bp * sizeof(*bp_info); >>> if (size > 0) { >>> bp_info = kmalloc(size, GFP_KERNEL); >>> if (!bp_info) { >> >> >> IMHO the common pattern for kmalloc is >> bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL); >> >> i can not remember code with a check for size < 0, i guess it is here >> to avoid an overflow ? since kmalloc takes a size_t argument this would cause >> a malloc failure an can be ignored. > > Shoudn't it be kcalloc? Or kmalloc_array, since zeroing is not necessary. Might be an idea for a new Coccinelle script, like - kmalloc (N * sizeof T, GFP) + kmalloc_array(N, sizeof T, GFP) Thanks, Paolo > > julia > >> >> >> just my 2 cents. >> re, >> wh >> > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 18.08.2016 11:48, schrieb Paolo Bonzini: > > > On 18/08/2016 11:02, Julia Lawall wrote: >> >> >> On Thu, 18 Aug 2016, walter harms wrote: >> >>> >>> >>> Am 17.08.2016 20:06, schrieb SF Markus Elfring: >>>> From: Markus Elfring <elfring@users.sourceforge.net> >>>> Date: Wed, 17 Aug 2016 18:29:04 +0200 >>>> >>>> Replace the specification of data structures by pointer dereferences >>>> to make the corresponding size determination a bit safer according to >>>> the Linux coding style convention. >>>> >>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >>>> --- >>>> arch/s390/kvm/guestdbg.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c >>>> index d1f8241..b68db4b 100644 >>>> --- a/arch/s390/kvm/guestdbg.c >>>> +++ b/arch/s390/kvm/guestdbg.c >>>> @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>> else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT) >>>> return -EINVAL; >>>> >>>> - size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint); >>>> + size = dbg->arch.nr_hw_bp * sizeof(*bp_data); >>>> bp_data = kmalloc(size, GFP_KERNEL); >>>> if (!bp_data) { >>>> ret = -ENOMEM; >>>> @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>> } >>>> } >>>> >>>> - size = nr_wp * sizeof(struct kvm_hw_wp_info_arch); >>>> + size = nr_wp * sizeof(*wp_info); >>>> if (size > 0) { >>>> wp_info = kmalloc(size, GFP_KERNEL); >>>> if (!wp_info) { >>>> @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>> goto error; >>>> } >>>> } >>>> - size = nr_bp * sizeof(struct kvm_hw_bp_info_arch); >>>> + size = nr_bp * sizeof(*bp_info); >>>> if (size > 0) { >>>> bp_info = kmalloc(size, GFP_KERNEL); >>>> if (!bp_info) { >>> >>> >>> IMHO the common pattern for kmalloc is >>> bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL); >>> >>> i can not remember code with a check for size < 0, i guess it is here >>> to avoid an overflow ? since kmalloc takes a size_t argument this would cause >>> a malloc failure an can be ignored. >> >> Shoudn't it be kcalloc? > > Or kmalloc_array, since zeroing is not necessary. Might be an idea for > a new Coccinelle script, like > > - kmalloc (N * sizeof T, GFP) > + kmalloc_array(N, sizeof T, GFP) > my personal taste is to stay close to the libc functions. technical there is no difference static inline void *kcalloc(size_t n, size_t size, gfp_t flags) { return kmalloc_array(n, size, flags | __GFP_ZERO); } and i do not see any time critical things here, re, wh > Thanks, > > Paolo > >> >> julia >> >>> >>> >>> just my 2 cents. >>> re, >>> wh >>> >> > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/08/2016 12:52, walter harms wrote: > > > Am 18.08.2016 11:48, schrieb Paolo Bonzini: >> >> >> On 18/08/2016 11:02, Julia Lawall wrote: >>> >>> >>> On Thu, 18 Aug 2016, walter harms wrote: >>> >>>> >>>> >>>> Am 17.08.2016 20:06, schrieb SF Markus Elfring: >>>>> From: Markus Elfring <elfring@users.sourceforge.net> >>>>> Date: Wed, 17 Aug 2016 18:29:04 +0200 >>>>> >>>>> Replace the specification of data structures by pointer dereferences >>>>> to make the corresponding size determination a bit safer according to >>>>> the Linux coding style convention. >>>>> >>>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >>>>> --- >>>>> arch/s390/kvm/guestdbg.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c >>>>> index d1f8241..b68db4b 100644 >>>>> --- a/arch/s390/kvm/guestdbg.c >>>>> +++ b/arch/s390/kvm/guestdbg.c >>>>> @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>>> else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT) >>>>> return -EINVAL; >>>>> >>>>> - size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint); >>>>> + size = dbg->arch.nr_hw_bp * sizeof(*bp_data); >>>>> bp_data = kmalloc(size, GFP_KERNEL); >>>>> if (!bp_data) { >>>>> ret = -ENOMEM; >>>>> @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>>> } >>>>> } >>>>> >>>>> - size = nr_wp * sizeof(struct kvm_hw_wp_info_arch); >>>>> + size = nr_wp * sizeof(*wp_info); >>>>> if (size > 0) { >>>>> wp_info = kmalloc(size, GFP_KERNEL); >>>>> if (!wp_info) { >>>>> @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>>> goto error; >>>>> } >>>>> } >>>>> - size = nr_bp * sizeof(struct kvm_hw_bp_info_arch); >>>>> + size = nr_bp * sizeof(*bp_info); >>>>> if (size > 0) { >>>>> bp_info = kmalloc(size, GFP_KERNEL); >>>>> if (!bp_info) { >>>> >>>> >>>> IMHO the common pattern for kmalloc is >>>> bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL); >>>> >>>> i can not remember code with a check for size < 0, i guess it is here >>>> to avoid an overflow ? since kmalloc takes a size_t argument this would cause >>>> a malloc failure an can be ignored. >>> >>> Shoudn't it be kcalloc? >> >> Or kmalloc_array, since zeroing is not necessary. Might be an idea for >> a new Coccinelle script, like >> >> - kmalloc (N * sizeof T, GFP) >> + kmalloc_array(N, sizeof T, GFP) >> > > > my personal taste is to stay close to the libc functions. > technical there is no difference > > static inline void *kcalloc(size_t n, size_t size, gfp_t flags) > { > return kmalloc_array(n, size, flags | __GFP_ZERO); > } > > and i do not see any time critical things here, This is _not_ premature optimization. (k)calloc tells the reader that it's safe not to initialize part of the array. kmalloc_array says the opposite. Using the right function adds important hints in the code---which unlike comments cannot get stale without also introducing visible bugs. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 18 Aug 2016 12:55:11 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > This is _not_ premature optimization. (k)calloc tells the reader that > it's safe not to initialize part of the array. kmalloc_array says the > opposite. Using the right function adds important hints in the > code---which unlike comments cannot get stale without also introducing > visible bugs. Ack. I'd accept a patch changing this to use kmalloc_array. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Or kmalloc_array, since zeroing is not necessary. Might be an idea for > a new Coccinelle script, like > > - kmalloc (N * sizeof T, GFP) > + kmalloc_array(N, sizeof T, GFP) I have picked your idea up. The corresponding script for the semantic patch language became longer than your general suggestion (if additional source code control flow aspects are integrated). Would it make sense to check any more function combinations in a similar way? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
----- Original Message ----- > From: "SF Markus Elfring" <elfring@users.sourceforge.net> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: "Julia Lawall" <julia.lawall@lip6.fr>, "walter harms" <wharms@bfs.de>, kvm@vger.kernel.org, > linux-s390@vger.kernel.org, "Christian Bornträger" <borntraeger@de.ibm.com>, "Cornelia Huck" > <cornelia.huck@de.ibm.com>, "David Hildenbrand" <dahi@linux.vnet.ibm.com>, "Heiko Carstens" > <heiko.carstens@de.ibm.com>, "Martin Schwidefsky" <schwidefsky@de.ibm.com>, "Radim Krčmář" <rkrcmar@redhat.com>, > "LKML" <linux-kernel@vger.kernel.org>, kernel-janitors@vger.kernel.org > Sent: Wednesday, August 24, 2016 2:10:13 PM > Subject: Re: Replacing specific kmalloc() calls by kmalloc_array()? > > > Or kmalloc_array, since zeroing is not necessary. Might be an idea for > > a new Coccinelle script, like > > > > - kmalloc (N * sizeof T, GFP) > > + kmalloc_array(N, sizeof T, GFP) > > I have picked your idea up. The corresponding script for the semantic > patch language became longer than your general suggestion > (if additional source code control flow aspects are integrated). > > Would it make sense to check any more function combinations > in a similar way? I don't know :) but I'm interested in seeing the semantic patch! Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c index d1f8241..b68db4b 100644 --- a/arch/s390/kvm/guestdbg.c +++ b/arch/s390/kvm/guestdbg.c @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT) return -EINVAL; - size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint); + size = dbg->arch.nr_hw_bp * sizeof(*bp_data); bp_data = kmalloc(size, GFP_KERNEL); if (!bp_data) { ret = -ENOMEM; @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, } } - size = nr_wp * sizeof(struct kvm_hw_wp_info_arch); + size = nr_wp * sizeof(*wp_info); if (size > 0) { wp_info = kmalloc(size, GFP_KERNEL); if (!wp_info) { @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, goto error; } } - size = nr_bp * sizeof(struct kvm_hw_bp_info_arch); + size = nr_bp * sizeof(*bp_info); if (size > 0) { bp_info = kmalloc(size, GFP_KERNEL); if (!bp_info) {