[PATCH] xen/many: xfree() can tolerate NULL pointers

[PATCH] xen/many: xfree() can tolerate NULL pointers

[Andrew Cooper]

Replace instances of "if ( p ) xfree(p)" with just "xfree(p)"

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

This was from some experimentation with semantic patches.  'spatch' can't
currently parse some of our macros (e.g. XEN_GUEST_HANDLE()), which cases it
to skip large numbers of functions in the codebase
---
 xen/arch/x86/cpu/mcheck/mctelem.c |    3 +--
 xen/arch/x86/mm/hap/hap.c         |    3 +--
 xen/common/kexec.c                |    3 +--
 xen/common/sched_credit2.c        |    3 +--
 xen/common/sched_sedf.c           |    3 +--
 xen/common/schedule.c             |    5 +----
 xen/xsm/flask/ss/policydb.c       |    8 ++++----
 7 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mctelem.c b/xen/arch/x86/cpu/mcheck/mctelem.c
index b8da465..95e83c5 100644
--- a/xen/arch/x86/cpu/mcheck/mctelem.c
+++ b/xen/arch/x86/cpu/mcheck/mctelem.c
@@ -260,8 +260,7 @@ void __init mctelem_init(unsigned int datasz)
 	if ((mctctl.mctc_elems = xmalloc_array(struct mctelem_ent,
 	    MC_NENT)) == NULL ||
 	    (datarr = xmalloc_bytes(MC_NENT * datasz)) == NULL) {
-		if (mctctl.mctc_elems)
-			xfree(mctctl.mctc_elems);
+		xfree(mctctl.mctc_elems);
 		printk("Allocations for MCA telemetry failed\n");
 		return;
 	}
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index abf3d7a..97f5168 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -168,8 +168,7 @@ int hap_track_dirty_vram(struct domain *d,
                                   p2m_ram_logdirty, p2m_ram_rw);
     }
 out:
-    if ( dirty_bitmap )
-        xfree(dirty_bitmap);
+    xfree(dirty_bitmap);
 
     return rc;
 }
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 2239ee8..d4c11cd 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -454,8 +454,7 @@ static int kexec_init_cpu_notes(const unsigned long cpu)
         spin_unlock(&crash_notes_lock);
         /* Always return ok, because whether we successfully allocated or not,
          * another CPU has successfully allocated. */
-        if ( note )
-            xfree(note);
+        xfree(note);
     }
     else
     {
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 1ca521b..ad0a5d4 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -2138,8 +2138,7 @@ csched2_deinit(const struct scheduler *ops)
     struct csched2_private *prv;
 
     prv = CSCHED2_PRIV(ops);
-    if ( prv != NULL )
-        xfree(prv);
+    xfree(prv);
 }
 
 
diff --git a/xen/common/sched_sedf.c b/xen/common/sched_sedf.c
index 7c80bad..c4f4b60 100644
--- a/xen/common/sched_sedf.c
+++ b/xen/common/sched_sedf.c
@@ -729,8 +729,7 @@ static void sedf_deinit(const struct scheduler *ops)
     struct sedf_priv_info *prv;
 
     prv = SEDF_PRIV(ops);
-    if ( prv != NULL )
-        xfree(prv);
+    xfree(prv);
 }
 
 
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 6285a6e..b73177f 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -260,10 +260,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
         if ( vcpu_priv[v->vcpu_id] == NULL )
         {
             for_each_vcpu ( d, v )
-            {
-                if ( vcpu_priv[v->vcpu_id] != NULL )
-                    xfree(vcpu_priv[v->vcpu_id]);
-            }
+                xfree(vcpu_priv[v->vcpu_id]);
             xfree(vcpu_priv);
             SCHED_OP(c->sched, free_domdata, domdata);
             return -ENOMEM;
diff --git a/xen/xsm/flask/ss/policydb.c b/xen/xsm/flask/ss/policydb.c
index 50b2c78..b88ea56 100644
--- a/xen/xsm/flask/ss/policydb.c
+++ b/xen/xsm/flask/ss/policydb.c
@@ -682,17 +682,17 @@ void policydb_destroy(struct policydb *p)
 
     for ( tr = p->role_tr; tr; tr = tr->next )
     {
-        if ( ltr ) xfree(ltr);
+        xfree(ltr);
         ltr = tr;
     }
-    if ( ltr ) xfree(ltr);
+    xfree(ltr);
 
     for ( ra = p->role_allow; ra; ra = ra -> next )
     {
-        if ( lra ) xfree(lra);
+        xfree(lra);
         lra = ra;
     }
-    if ( lra ) xfree(lra);
+    xfree(lra);
 
     for ( rt = p->range_tr; rt; rt = rt -> next )
     {
-- 
1.7.10.4

Re: [PATCH] xen/many: xfree() can tolerate NULL pointers

[Ian Campbell]

On Mon, 2015-01-19 at 10:42 +0000, Andrew Cooper wrote:
> Replace instances of "if ( p ) xfree(p)" with just "xfree(p)"
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> ---
> 
> This was from some experimentation with semantic patches.  'spatch' can't
> currently parse some of our macros (e.g. XEN_GUEST_HANDLE()), which cases it
> to skip large numbers of functions in the codebase

How annoying! (and surprising)

Anyway, could you include the spatch in the commit log, for completeness
and for future cargo culting ;-) (unless it's huge, I guess)

Ian.

Re: [PATCH] xen/many: xfree() can tolerate NULL pointers

[Andrew Cooper]

On 19/01/15 10:54, Ian Campbell wrote:
> On Mon, 2015-01-19 at 10:42 +0000, Andrew Cooper wrote:
>> Replace instances of "if ( p ) xfree(p)" with just "xfree(p)"
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>
>> ---
>>
>> This was from some experimentation with semantic patches.  'spatch' can't
>> currently parse some of our macros (e.g. XEN_GUEST_HANDLE()), which cases it
>> to skip large numbers of functions in the codebase
> How annoying! (and surprising)

It also can't parse the "case $X ... $Y:" syntax if the spaces around
the ellipsis is missing.  This is a little more understandable as this
gccism does introduce an ambiguity into the grammar, insofar that 1...3
is not a valid floating point constant.

We have a number of uses with #defined numbers uses without spaces. 
This turns out to be safe only because the preprocessor replacement puts
spaces around replaced tokens.

>
> Anyway, could you include the spatch in the commit log, for completeness
> and for future cargo culting ;-) (unless it's huge, I guess)

It was tiny, but sadly scummed to a `git clean`.  I can't even recall
which of the many kfree() variants I ended up modifying (s/k/x/), but
there is a huge library of generic and Linux specific semantic patches
at https://github.com/coccinelle/coccinellery

~Andrew

Re: [PATCH] xen/many: xfree() can tolerate NULL pointers

[Ian Campbell]

On Mon, 2015-01-19 at 11:12 +0000, Andrew Cooper wrote:
> On 19/01/15 10:54, Ian Campbell wrote:
> > Anyway, could you include the spatch in the commit log, for completeness
> > and for future cargo culting ;-) (unless it's huge, I guess)
> 
> It was tiny, but sadly scummed to a `git clean`.  I can't even recall
> which of the many kfree() variants I ended up modifying (s/k/x/),

No worries, next time ;-)

Re: [PATCH] xen/many: xfree() can tolerate NULL pointers

[Jan Beulich]

>>> On 19.01.15 at 12:12, <andrew.cooper3@citrix.com> wrote:
> On 19/01/15 10:54, Ian Campbell wrote:
>> On Mon, 2015-01-19 at 10:42 +0000, Andrew Cooper wrote:
>>> Replace instances of "if ( p ) xfree(p)" with just "xfree(p)"
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Keir Fraser <keir@xen.org>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Tim Deegan <tim@xen.org>
>>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>
>>> ---
>>>
>>> This was from some experimentation with semantic patches.  'spatch' can't
>>> currently parse some of our macros (e.g. XEN_GUEST_HANDLE()), which cases it
>>> to skip large numbers of functions in the codebase
>> How annoying! (and surprising)
> 
> It also can't parse the "case $X ... $Y:" syntax if the spaces around
> the ellipsis is missing.  This is a little more understandable as this
> gccism does introduce an ambiguity into the grammar, insofar that 1...3
> is not a valid floating point constant.
> 
> We have a number of uses with #defined numbers uses without spaces. 
> This turns out to be safe only because the preprocessor replacement puts
> spaces around replaced tokens.

Spaces? That could end up being problematic (e.g. when
subsequently making the resulting token sequence subject to a
# operator). Afaik the preprocessor simply doesn't concatenate
adjacent tokens after expansion, unless told so by the ##
operator.

Jan

Re: [PATCH] xen/many: xfree() can tolerate NULL pointers

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] xen/many: xfree() can tolerate NULL pointers"):
> On Mon, 2015-01-19 at 10:42 +0000, Andrew Cooper wrote:
> > This was from some experimentation with semantic patches.  'spatch' can't
> > currently parse some of our macros (e.g. XEN_GUEST_HANDLE()), which cases it
> > to skip large numbers of functions in the codebase
> 
> How annoying! (and surprising)
> 
> Anyway, could you include the spatch in the commit log, for completeness
> and for future cargo culting ;-) (unless it's huge, I guess)

I think the spatch is the preferred form for modification of an
spatch-generated patch.  Therefore an spatch-generated patch should
not be accepted without the spatch.

(I would decline to add my own s-o-b to such a thing.)

Ian.

Re: [PATCH] xen/many: xfree() can tolerate NULL pointers

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] xen/many: xfree() can tolerate NULL pointers"):
> On Mon, 2015-01-19 at 11:12 +0000, Andrew Cooper wrote:
> > On 19/01/15 10:54, Ian Campbell wrote:
> > > Anyway, could you include the spatch in the commit log, for completeness
> > > and for future cargo culting ;-) (unless it's huge, I guess)
> > 
> > It was tiny, but sadly scummed to a `git clean`.  I can't even recall
> > which of the many kfree() variants I ended up modifying (s/k/x/),
> 
> No worries, next time ;-)

Umm, further to my last mail, we (the Free Software world in general)
should treat this the same way we would any other case where the
source code has been genuinely lost.  We start editing the output
file, and treat the results as the new source code.

(Something can only be the preferred form for modification if it
actually exists.)

Ian.

Re: [PATCH] xen/many: xfree() can tolerate NULL pointers

[Jan Beulich]

>>> On 19.01.15 at 12:47, <Ian.Jackson@eu.citrix.com> wrote:
> Ian Campbell writes ("Re: [PATCH] xen/many: xfree() can tolerate NULL 
> pointers"):
>> On Mon, 2015-01-19 at 11:12 +0000, Andrew Cooper wrote:
>> > On 19/01/15 10:54, Ian Campbell wrote:
>> > > Anyway, could you include the spatch in the commit log, for completeness
>> > > and for future cargo culting ;-) (unless it's huge, I guess)
>> > 
>> > It was tiny, but sadly scummed to a `git clean`.  I can't even recall
>> > which of the many kfree() variants I ended up modifying (s/k/x/),
>> 
>> No worries, next time ;-)
> 
> Umm, further to my last mail, we (the Free Software world in general)
> should treat this the same way we would any other case where the
> source code has been genuinely lost.  We start editing the output
> file, and treat the results as the new source code.
> 
> (Something can only be the preferred form for modification if it
> actually exists.)

So is this an active NAK to the patch then? Afaic the patch
description doesn't even mention the origin being a spatch, and
hence the change by itself seems fine to me, and consistent with
its description - it could easily have been the result of human
auditing of the code.

Jan

Re: [PATCH] xen/many: xfree() can tolerate NULL pointers

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] xen/many: xfree() can tolerate NULL pointers"):
> On 19.01.15 at 12:47, <Ian.Jackson@eu.citrix.com> wrote:
> > Umm, further to my last mail, we (the Free Software world in general)
> > should treat this the same way we would any other case where the
> > source code has been genuinely lost.  We start editing the output
> > file, and treat the results as the new source code.
> > 
> > (Something can only be the preferred form for modification if it
> > actually exists.)
> 
> So is this an active NAK to the patch then? Afaic the patch
> description doesn't even mention the origin being a spatch, and
> hence the change by itself seems fine to me, and consistent with
> its description - it could easily have been the result of human
> auditing of the code.

I should have been clearer.  My earlier message was a "nak until we
have the spatch too" and my most recent message was me withdrawing
that.

So I have no objection.  And this is a change I approve of, so fwiw

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

But I was also grumbling a bit about lost source code.  Next time we
could do a bit better.

Ian.