Re: [PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()"

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-07-16 at 23:30 +0100, Wei Liu wrote:
> On Thu, Jul 16, 2015 at 10:53:02PM +0100, Andrew Cooper wrote:
> > On 16/07/2015 21:47, Wei Liu wrote:
> > > On Thu, Jul 16, 2015 at 08:18:31PM +0100, Andrew Cooper wrote:
> > >> c/s e316316 "xl: Rewrite trim()" uses the wrong indirection of
> > >> 'output', causing memory corruption for all callers.
> > >>
> > >> Introduce a new local variable, making the code more obviously
> > >> correct.
> > >>
> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > >> CC: Ian Campbell <Ian.Campbell@citrix.com>
> > >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > >> CC: Wei Liu <wei.liu2@citrix.com>
> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> > >
> > > One nit below.
> > >
> > >> ---
> > >>  tools/libxl/xl_cmdimpl.c |    8 +++++---
> > >>  1 file changed, 5 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > >> index 37d4af6..0cc9f8b 100644
> > >> --- a/tools/libxl/xl_cmdimpl.c
> > >> +++ b/tools/libxl/xl_cmdimpl.c
> > >> @@ -663,10 +663,12 @@ static void trim(char_predicate_t predicate, const char *input, char **output)
> > >>          ;
> > >>  
> > >>      size_t len_nonnull = after - first;
> > >> +    char *result = xmalloc(len_nonnull + 1);
> > >>  
> > > Can you move the declaration of result to the beginning of this
> > > function?
> > 
> > I can, but why in particular?  It is adjacent to the declaration of
> > len_nonnull.
> > 
> 
> C90 forbids this. Not sure how much we care about that and I'm probably
> bicksheding too much.

We made an explicit exception for this in libxl, via the use of
-Wno-declaration-after-statement.

IIRC because some of the GC macros require it, but it has since become
allowed more generally (although not documented it seems).

In this particular case having result declared right after len_nonnull
makes sense.

I was about to Ack + apply but I see Ian beat me to it.

Ian.