Here needs a NULL check.
Issue found by coccinelle.
Signed-off-by: Qiujun Huang <[email protected]>
---
arch/powerpc/platforms/powernv/opal.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2b3dfd0b6cdd..908d749bcef5 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -801,16 +801,19 @@ static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
static int opal_add_one_export(struct kobject *parent, const char *export_name,
struct device_node *np, const char *prop_name)
{
- struct bin_attribute *attr = NULL;
- const char *name = NULL;
+ struct bin_attribute *attr;
+ const char *name;
u64 vals[2];
int rc;
rc = of_property_read_u64_array(np, prop_name, &vals[0], 2);
if (rc)
- goto out;
+ return rc;
attr = kzalloc(sizeof(*attr), GFP_KERNEL);
+ if (!attr)
+ return -ENOMEM;
+
name = kstrdup(export_name, GFP_KERNEL);
if (!name) {
rc = -ENOMEM;
--
2.17.1
> Here needs a NULL check.
I find this change description questionable
(despite of a reasonable patch subject).
> Issue found by coccinelle.
Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
be nicer?
> ---
Will a patch change log be helpful here?
Regards,
Markus
On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <[email protected]> wrote:
>
> > Here needs a NULL check.
quite obvious?
>
> I find this change description questionable
> (despite of a reasonable patch subject).
>
>
> > Issue found by coccinelle.
>
> Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
> be nicer?
Yeah, but I think It was enough.
>
>
> > ---
>
> Will a patch change log be helpful here?
I realized I should write some change log, and the change log was meaningless.
So I left it blank.
>
> Regards,
> Markus
On Mon, Apr 6, 2020 at 11:15 AM Qiujun Huang <[email protected]> wrote:
>
> On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <[email protected]> wrote:
> >
> > > Here needs a NULL check.
> quite obvious?
> >
> > I find this change description questionable
> > (despite of a reasonable patch subject).
> >
> >
> > > Issue found by coccinelle.
> >
> > Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
> > be nicer?
> Yeah, but I think It was enough.
I didn't know we had that script in the kernel tree so I think it's a
good to mention that you used it. It might even help idiots like me
who write this sort of bug.
> > Will a patch change log be helpful here?
> I realized I should write some change log, and the change log was meaningless.
> So I left it blank.
The changelog is fine IMO. The point of a changelog is to tell a
reader doing git archeology why a change happened and this is
sufficent for that.
Reviewed-by: Oliver O'Halloran <[email protected]>
On Mon, Apr 6, 2020 at 5:01 PM Oliver O'Halloran <[email protected]> wrote:
>
> On Mon, Apr 6, 2020 at 11:15 AM Qiujun Huang <[email protected]> wrote:
> >
> > On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <[email protected]> wrote:
> > >
> > > > Here needs a NULL check.
> > quite obvious?
> > >
> > > I find this change description questionable
> > > (despite of a reasonable patch subject).
> > >
> > >
> > > > Issue found by coccinelle.
> > >
> > > Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
> > > be nicer?
> > Yeah, but I think It was enough.
>
> I didn't know we had that script in the kernel tree so I think it's a
> good to mention that you used it. It might even help idiots like me
> who write this sort of bug.
Yes, I will resend the patch. Thanks :-)
>
> > > Will a patch change log be helpful here?
> > I realized I should write some change log, and the change log was meaningless.
> > So I left it blank.
>
> The changelog is fine IMO. The point of a changelog is to tell a
> reader doing git archeology why a change happened and this is
> sufficent for that.
Get that.
>
> Reviewed-by: Oliver O'Halloran <[email protected]>
>>>> Here needs a NULL check.
>> quite obvious?
I suggest to consider another fine-tuning for the wording also around
such “obvious” programming items.
>>> I find this change description questionable
>>> (despite of a reasonable patch subject).
I got further development concerns.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n129
* Were changes mixed for different issues according to the diff code?
* I find it safer here to split specific changes into separate update steps
for a small patch series.
* Will the addition of the desired null pointer check qualify for
the specification of the tag “Fixes”?
>>> Will a patch change log be helpful here?
>> I realized I should write some change log, and the change log was meaningless.
Will any more adjustments happen for the discussed update suggestion
after the third patch version?
> The changelog is fine IMO. The point of a changelog is to tell a
> reader doing git archeology why a change happened and this is
> sufficent for that.
We might stumble on a different understanding for the affected “change logs”.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n751
Would you like to follow the patch evolution a bit easier?
Regards,
Markus
On Mon, Apr 6, 2020 at 6:02 PM Markus Elfring <[email protected]> wrote:
>
> >>>> Here needs a NULL check.
> >> quite obvious?
>
> I suggest to consider another fine-tuning for the wording also around
> such “obvious” programming items.
>
>
> >>> I find this change description questionable
> >>> (despite of a reasonable patch subject).
>
> I got further development concerns.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n129
>
> * Were changes mixed for different issues according to the diff code?
>
> * I find it safer here to split specific changes into separate update steps
> for a small patch series.
>
> * Will the addition of the desired null pointer check qualify for
> the specification of the tag “Fixes”?
>
>
> >>> Will a patch change log be helpful here?
> >> I realized I should write some change log, and the change log was meaningless.
>
> Will any more adjustments happen for the discussed update suggestion
> after the third patch version?
>
>
> > The changelog is fine IMO. The point of a changelog is to tell a
> > reader doing git archeology why a change happened and this is
> > sufficent for that.
>
> We might stumble on a different understanding for the affected “change logs”.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n751
>
> Would you like to follow the patch evolution a bit easier?
>
> Regards,
> Markus
Thanks for the reply.
I should study the documentation first.
BTW, happy new week