Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

[Markus Elfring]

> 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

Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

[Qiujun Huang]

On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <Markus.Elfring@web.de> 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

Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

[Oliver O'Halloran]

On Mon, Apr 6, 2020 at 11:15 AM Qiujun Huang <hqjagain@gmail.com> wrote:
>
> On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <Markus.Elfring@web.de> 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 <oohall@gmail.com>

Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

[Qiujun Huang]

On Mon, Apr 6, 2020 at 5:01 PM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> On Mon, Apr 6, 2020 at 11:15 AM Qiujun Huang <hqjagain@gmail.com> wrote:
> >
> > On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <Markus.Elfring@web.de> 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 <oohall@gmail.com>

Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

[Markus Elfring]

>>>> 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

Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

[Qiujun Huang]

On Mon, Apr 6, 2020 at 6:02 PM Markus Elfring <Markus.Elfring@web.de> 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

[PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

[Qiujun Huang]

Here needs a NULL check.

Issue found by coccinelle.

Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
---
 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