From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
On 08/07/2017 03:52 AM, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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
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.
>> 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
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
>> 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
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
>>> 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
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
SF Markus Elfring <[email protected]> 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?
>>
>> 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/
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
>> 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