[PATCH] block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[PATCH] block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 7 Aug 2017 12:37:01 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/ps3vram.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c
index e0e81cacd781..ba97d037279e 100644
--- a/drivers/block/ps3vram.c
+++ b/drivers/block/ps3vram.c
@@ -409,10 +409,8 @@ static int ps3vram_cache_init(struct ps3_system_bus_device *dev)
 	priv->cache.page_size = CACHE_PAGE_SIZE;
 	priv->cache.tags = kzalloc(sizeof(struct ps3vram_tag) *
 				   CACHE_PAGE_COUNT, GFP_KERNEL);
-	if (priv->cache.tags == NULL) {
-		dev_err(&dev->core, "Could not allocate cache tags\n");
+	if (!priv->cache.tags)
 		return -ENOMEM;
-	}
 
 	dev_info(&dev->core, "Created ram cache: %d entries, %d KiB each\n",
 		CACHE_PAGE_COUNT, CACHE_PAGE_SIZE / 1024);
-- 
2.13.4

Re: [PATCH] block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[Geoff Levand]

On 08/07/2017 03:52 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 7 Aug 2017 12:37:01 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.

NACK

When a user asks me for help I would certainly like to get 
'Could not allocate cache tags' as apposed to nothing, since
the return value of ps3vram_cache_init() is not checked.

If you want to make an improvement please add a check for
success of ps3vram_cache_init() in ps3vram_probe().

-Geoff

Re: [PATCH] block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[Joe Perches]

On Mon, 2017-08-07 at 08:10 -0700, Geoff Levand wrote:
> On 08/07/2017 03:52 AM, SF Markus Elfring wrote:
> > Omit an extra message for a memory allocation failure in this function.
> NACK
> 
> When a user asks me for help I would certainly like to get 
> 'Could not allocate cache tags' as apposed to nothing, since
> the return value of ps3vram_cache_init() is not checked.

You still get a dump_stack on alloc failure.

Re: block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[SF Markus Elfring]

>> Omit an extra message for a memory allocation failure in this function.
>>
>> This issue was detected by using the Coccinelle software.
> 
> NACK
> 
> When a user asks me for help I would certainly like to get 
> 'Could not allocate cache tags' as apposed to nothing,

Do you find the default allocation failure report insufficient?


> since the return value of ps3vram_cache_init() is not checked.

Are there any more update candidates to consider for better exception handling?

Regards,
Markus

Re: block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[Geoff Levand]

On 08/07/2017 09:27 AM, SF Markus Elfring wrote:
>>> Omit an extra message for a memory allocation failure in this function.
>>>
>>> This issue was detected by using the Coccinelle software.
>>
>> NACK
>>
>> When a user asks me for help I would certainly like to get 
>> 'Could not allocate cache tags' as apposed to nothing,
> 
> Do you find the default allocation failure report insufficient?

The default is OK.  I didn't consider one would be triggered by
the kzalloc failure.

>> since the return value of ps3vram_cache_init() is not checked.
> 
> Are there any more update candidates to consider for better exception handling?

The return of ps3vram_cache_init() should be checked.  Feel
free to propose others.

-Geoff

Re: block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[SF Markus Elfring]

>> Do you find the default allocation failure report insufficient?
> 
> The default is OK.

Thanks for this information.


> I didn't consider one would be triggered by the kzalloc failure.

Do you reconsider any special system settings for further
software evolution then?

Regards,
Markus

Re: block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[Geoff Levand]

On 08/07/2017 11:34 AM, SF Markus Elfring wrote:
>> I didn't consider one would be triggered by the kzalloc failure.
> 
> Do you reconsider any special system settings for further
> software evolution then?

Sorry, I don't quite understand your question.

I think your original patch is OK, and I would appreciate if
you added a check for failure of ps3vram_cache_init() in
ps3vram_probe().  If you decide not to add that check I'll
create a patch for it later.

If this doesn't answer your question, could you please
rephrase it?

-Geoff

Re: block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[SF Markus Elfring]

>>> I didn't consider one would be triggered by the kzalloc failure.
>>
>> Do you reconsider any special system settings for further
>> software evolution then?
> 
> Sorry, I don't quite understand your question.

Do you try to configure the Linux error reporting to any special needs?


> I think your original patch is OK,

How does this feedback fit to the initial response “Not Applicable”?
https://patchwork.ozlabs.org/patch/798575/


> and I would appreciate if you added a check for failure of ps3vram_cache_init()
> in ps3vram_probe().

I unsure if this adjustment will need more software updates.


> If you decide not to add that check I'll create a patch for it later.

I am curious on who will pick this update candidate up as the next improvement.
Have you got any preferences for the exception handling there?

Regards,
Markus

Re: block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[Geoff Levand]

On 08/07/2017 12:04 PM, SF Markus Elfring wrote:
>> I think your original patch is OK,
> 
> How does this feedback fit to the initial response “Not Applicable”?
> https://patchwork.ozlabs.org/patch/798575/

I submitted your patch and a fix to ps3vram_probe() with
the other patches in my queue.  Thanks for your contribution.

-Geoff

Re: block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[SF Markus Elfring]

>> https://patchwork.ozlabs.org/patch/798575/
> 
> I submitted your patch

Thanks for your constructive feedback.
https://patchwork.ozlabs.org/patch/798850/


> and a fix to ps3vram_probe() with the other patches in my queue.

I find it nice that you picked this change opportunity up after
a bit of discussion (before an other developer would eventually
have tackled it also).

“Check return of ps3vram_cache_init”
https://patchwork.ozlabs.org/patch/798853/

1. Unfortunately, I find that this specific update suggestion does not fit
   to the Linux coding style convention.

   “…
   Do not unnecessarily use braces where a single statement will do.
   …”

2. How do you think about to use the check “if (error)” instead?

3. Will an additional commit description be useful?

Regards,
Markus

Re: block/ps3vram: Delete an error message for a failed memory allocation in ps3vram_cache_init()

[Michael Ellerman]

SF Markus Elfring <elfring@users.sourceforge.net> writes:

>>>> I didn't consider one would be triggered by the kzalloc failure.
>>>
>>> Do you reconsider any special system settings for further
>>> software evolution then?
>>=20
>> Sorry, I don't quite understand your question.
>
> Do you try to configure the Linux error reporting to any special needs?
>
>
>> I think your original patch is OK,
>
> How does this feedback fit to the initial response =E2=80=9CNot Applicabl=
e=E2=80=9D?
> https://patchwork.ozlabs.org/patch/798575/

That comes from me, and means "I can't apply this patch", because it's
not a powerpc patch.

Looking at the maintainers output though maybe that is meant to go via
the powerpc tree.

cheers