Re: [PATCH v2] staging: atomisp: use kstrdup to replace kmalloc and memcpy
On 10 July 2017 at 01:22, Sakari Ailus wrote:
> On Sun, Jul 09, 2017 at 05:56:15PM +0530, hari prasath wrote:
>> On 8 July 2017 at 16:31, Sakari Ailus wrote:
>> > Hi Hari,
>> >
>> > On Fri, Jul 07, 2017 at 08:15:21PM +0530, Hari Prasath wrote:
>> >> kstrdup kernel primitive can be used to replace kmalloc followed by
>> >> string copy. This was reported by coccinelle tool
>> >>
>> >> Signed-off-by: Hari Prasath
>> >> ---
>> >> .../media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 10
>> >> +++---
>> >> 1 file changed, 3 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git
>> >> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
>> >> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
>> >> index 34cc56f..68db87b 100644
>> >> --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
>> >> +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
>> >> @@ -144,14 +144,10 @@ sh_css_load_blob_info(const char *fw, const struct
>> >> ia_css_fw_info *bi, struct ia
>> >> )
>> >> {
>> >> char *namebuffer;
>> >> - int namelength = (int)strlen(name);
>> >> -
>> >> - namebuffer = (char *) kmalloc(namelength + 1, GFP_KERNEL);
>> >> - if (namebuffer == NULL)
>> >> - return IA_CSS_ERR_CANNOT_ALLOCATE_MEMORY;
>> >> -
>> >> - memcpy(namebuffer, name, namelength + 1);
>> >>
>> >> + namebuffer = kstrdup(name, GFP_KERNEL);
>> >> + if (!namebuffer)
>> >> + return -ENOMEM;
>> >
>> > The patch also changes the return value in error cases. I believe the
>> > caller(s) expect to get errors in the IA_CCS_ERR_* range.
>>
>> Hi,
>>
>> In this particular case, the calling function just checks if it's not
>> success defined by a enum. I think returning -ENOMEM would not effect,
>> at least in this case.
>
> It might not, but the function now returns both negative Posix and positive
> CSS error codes. The CSS error codes could well be converted to Posix but
> it should be done consistently and preferrably in a separate patch.
Hi Sakari, Thanks for your comments. I will stick with just replacing
with kstrdup and retain the original error return value. I will send a
v3.
Regards,
Hari
>
> --
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
Regards,
G.E.Hari Prasath
[PATCHv3] staging: atomisp: use kstrdup to replace kmalloc and memcpy
kstrdup kernel primitive can be used to replace kmalloc followed by
string copy. This was reported by coccinelle tool.
Signed-off-by: Hari Prasath
---
v1: Replace kmalloc followed by memcpy with kmemdup. Based on
review comments from Alan Cox, this could better be done
using kstrdup.
v2: Replace kmalloc followed by memcpy by kstrdup in this case
as it essentially is a string copy.Review comment recieved
questioning the return value in case of error. Error value
returned should be what the calling function is expecting.
v3: Retain the original error value returned to the calling
function if kstrdup() fails.
---
.../staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
index 34cc56f..5d231ee 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
@@ -144,14 +144,10 @@ sh_css_load_blob_info(const char *fw, const struct
ia_css_fw_info *bi, struct ia
)
{
char *namebuffer;
- int namelength = (int)strlen(name);
- namebuffer = (char *) kmalloc(namelength + 1, GFP_KERNEL);
- if (namebuffer == NULL)
+ namebuffer = kstrdup(name, GFP_KERNEL);
+ if (!namebuffer)
return IA_CSS_ERR_CANNOT_ALLOCATE_MEMORY;
-
- memcpy(namebuffer, name, namelength + 1);
-
bd->name = fw_minibuffer[index].name = namebuffer;
} else {
bd->name = name;
--
2.10.0.GIT
Re: [PATCH v2] staging: atomisp: use kstrdup to replace kmalloc and memcpy
On 8 July 2017 at 16:31, Sakari Ailus wrote:
> Hi Hari,
>
> On Fri, Jul 07, 2017 at 08:15:21PM +0530, Hari Prasath wrote:
>> kstrdup kernel primitive can be used to replace kmalloc followed by
>> string copy. This was reported by coccinelle tool
>>
>> Signed-off-by: Hari Prasath
>> ---
>> .../media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 10
>> +++---
>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git
>> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
>> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
>> index 34cc56f..68db87b 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
>> @@ -144,14 +144,10 @@ sh_css_load_blob_info(const char *fw, const struct
>> ia_css_fw_info *bi, struct ia
>> )
>> {
>> char *namebuffer;
>> - int namelength = (int)strlen(name);
>> -
>> - namebuffer = (char *) kmalloc(namelength + 1, GFP_KERNEL);
>> - if (namebuffer == NULL)
>> - return IA_CSS_ERR_CANNOT_ALLOCATE_MEMORY;
>> -
>> - memcpy(namebuffer, name, namelength + 1);
>>
>> + namebuffer = kstrdup(name, GFP_KERNEL);
>> + if (!namebuffer)
>> + return -ENOMEM;
>
> The patch also changes the return value in error cases. I believe the
> caller(s) expect to get errors in the IA_CCS_ERR_* range.
Hi,
In this particular case, the calling function just checks if it's not
success defined by a enum. I think returning -ENOMEM would not effect,
at least in this case.
- Hari Prasath
>
>> bd->name = fw_minibuffer[index].name = namebuffer;
>> } else {
>> bd->name = name;
>
> --
> Regards,
>
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
Regards,
G.E.Hari Prasath
[PATCH v2] staging: atomisp: use kstrdup to replace kmalloc and memcpy
kstrdup kernel primitive can be used to replace kmalloc followed by
string copy. This was reported by coccinelle tool
Signed-off-by: Hari Prasath
---
.../media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
index 34cc56f..68db87b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
@@ -144,14 +144,10 @@ sh_css_load_blob_info(const char *fw, const struct
ia_css_fw_info *bi, struct ia
)
{
char *namebuffer;
- int namelength = (int)strlen(name);
-
- namebuffer = (char *) kmalloc(namelength + 1, GFP_KERNEL);
- if (namebuffer == NULL)
- return IA_CSS_ERR_CANNOT_ALLOCATE_MEMORY;
-
- memcpy(namebuffer, name, namelength + 1);
+ namebuffer = kstrdup(name, GFP_KERNEL);
+ if (!namebuffer)
+ return -ENOMEM;
bd->name = fw_minibuffer[index].name = namebuffer;
} else {
bd->name = name;
--
2.10.0.GIT
Re: [PATCH] staging: atomisp: replace kmalloc & memcpy with kmemdup
On 07-Jul-2017 5:25 PM, "Alan Cox" wrote: On Fri, 2017-07-07 at 17:20 +0530, Hari Prasath wrote: > kmemdup can be used to replace kmalloc followed by a memcpy.This was > pointed out by the coccinelle tool. And kstrdup could do the job even better I think ? > Yes & thanks for pointing me that. I will send a v2 version. -Hari
[PATCH] staging: atomisp: replace kmalloc & memcpy with kmemdup
kmemdup can be used to replace kmalloc followed by a memcpy.This was
pointed out by the coccinelle tool.
Signed-off-by: Hari Prasath
---
drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
index 34cc56f..58d4619 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
@@ -146,12 +146,10 @@ sh_css_load_blob_info(const char *fw, const struct
ia_css_fw_info *bi, struct ia
char *namebuffer;
int namelength = (int)strlen(name);
- namebuffer = (char *) kmalloc(namelength + 1, GFP_KERNEL);
+ namebuffer = (char *)kmemdup(name, namelength + 1, GFP_KERNEL);
if (namebuffer == NULL)
return IA_CSS_ERR_CANNOT_ALLOCATE_MEMORY;
- memcpy(namebuffer, name, namelength + 1);
-
bd->name = fw_minibuffer[index].name = namebuffer;
} else {
bd->name = name;
--
2.10.0.GIT
