diff mbox

fs/ext2: fix double quoted label

Message ID 20161116195543.6456-2-gael.portay@savoirfairelinux.com
State Changes Requested
Headers show

Commit Message

Gaël PORTAY Nov. 16, 2016, 7:55 p.m. UTC
Since the commit 6dd7bbb59134799ed3d7343f238b3b02592faebf, the label does
not need anymore to be quoted. Even worse it *must* not be simple-quoted,
unless the label will contain the double-quotes from the config variable
BR2_TARGET_ROOTFS_EXT2_LABEL.

Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
---
 fs/ext2/ext2.mk | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Arnout Vandecappelle Nov. 16, 2016, 10:58 p.m. UTC | #1
On 16-11-16 20:55, Gaël PORTAY wrote:
> Since the commit 6dd7bbb59134799ed3d7343f238b3b02592faebf, the label does
> not need anymore to be quoted. Even worse it *must* not be simple-quoted,
> unless the label will contain the double-quotes from the config variable
> BR2_TARGET_ROOTFS_EXT2_LABEL.
> 
> Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
> ---
>  fs/ext2/ext2.mk | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
> index 7417f81..49b44e4 100644
> --- a/fs/ext2/ext2.mk
> +++ b/fs/ext2/ext2.mk
> @@ -20,12 +20,8 @@ ifneq ($(strip $(BR2_TARGET_ROOTFS_EXT2_RESBLKS)),0)
>  EXT2_OPTS += -r $(BR2_TARGET_ROOTFS_EXT2_RESBLKS)
>  endif
>  
> -# Not qstrip-ing the variable, because it may contain spaces, but we must
> -# qstrip it when checking. Furthermore, we need to further quote it, so
> -# that the quotes do not get eaten by the echo statement when creating the
> -# fakeroot script
>  ifneq ($(call qstrip,$(BR2_TARGET_ROOTFS_EXT2_LABEL)),)
> -EXT2_OPTS += -l '$(BR2_TARGET_ROOTFS_EXT2_LABEL)'
> +EXT2_OPTS += -l $(BR2_TARGET_ROOTFS_EXT2_LABEL)

 We usually don't rely on the quotes added by Kconfig, because it's possible
that the variable is overridden on the command line like
make BR2_TARGET_ROOTFS_EXT2_LABEL="foo bar"

So it should be
EXT2_OPTS += -l '$(call qstrip,$(BR2_TARGET_ROOTFS_EXT2_LABEL))'

 Regards,
 Arnout

>  endif
>  
>  ROOTFS_EXT2_DEPENDENCIES = host-mke2img
>
Gaël PORTAY Nov. 17, 2016, 5:48 p.m. UTC | #2
Hi Arnout,

I am sorry, I saw your email shortly afterwards sending the v2...

On Wed, Nov 16, 2016 at 11:58:23PM +0100, Arnout Vandecappelle wrote:
> 
> 
> On 16-11-16 20:55, Gaël PORTAY wrote:
> > Since the commit 6dd7bbb59134799ed3d7343f238b3b02592faebf, the label does
> > not need anymore to be quoted. Even worse it *must* not be simple-quoted,
> > unless the label will contain the double-quotes from the config variable
> > BR2_TARGET_ROOTFS_EXT2_LABEL.
> > 
> > Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
> > ---
> >  fs/ext2/ext2.mk | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
> > index 7417f81..49b44e4 100644
> > --- a/fs/ext2/ext2.mk
> > +++ b/fs/ext2/ext2.mk
> > @@ -20,12 +20,8 @@ ifneq ($(strip $(BR2_TARGET_ROOTFS_EXT2_RESBLKS)),0)
> >  EXT2_OPTS += -r $(BR2_TARGET_ROOTFS_EXT2_RESBLKS)
> >  endif
> >  
> > -# Not qstrip-ing the variable, because it may contain spaces, but we must
> > -# qstrip it when checking. Furthermore, we need to further quote it, so
> > -# that the quotes do not get eaten by the echo statement when creating the
> > -# fakeroot script
> >  ifneq ($(call qstrip,$(BR2_TARGET_ROOTFS_EXT2_LABEL)),)
> > -EXT2_OPTS += -l '$(BR2_TARGET_ROOTFS_EXT2_LABEL)'
> > +EXT2_OPTS += -l $(BR2_TARGET_ROOTFS_EXT2_LABEL)
> 
>  We usually don't rely on the quotes added by Kconfig, because it's possible
> that the variable is overridden on the command line like
> make BR2_TARGET_ROOTFS_EXT2_LABEL="foo bar"
> 

For sure you are right, it was not not appropriate to use the Kconfig variable.

> So it should be
> EXT2_OPTS += -l '$(call qstrip,$(BR2_TARGET_ROOTFS_EXT2_LABEL))'
> 

As discuss with Thomas and Yann on the channel, we moved to this solution:

EXT2_LABEL := $(subst ",,$(BR2_TARGET_ROOTFS_EXT2_LABEL))
ifneq ($(EXT2_LABEL),)
EXT2_OPTS += -l "$(EXT2_LABEL)"
endif

... to not rely on the quotes of Kconfig (as you said) and to preserved the
spaces from the Kconfig variable ("BR   2016-11-rc2").

>  Regards,
>  Arnout
> 
> >  endif
> >  
> >  ROOTFS_EXT2_DEPENDENCIES = host-mke2img
> > 
> 
> -- 
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Regards,
Gaël
diff mbox

Patch

diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
index 7417f81..49b44e4 100644
--- a/fs/ext2/ext2.mk
+++ b/fs/ext2/ext2.mk
@@ -20,12 +20,8 @@  ifneq ($(strip $(BR2_TARGET_ROOTFS_EXT2_RESBLKS)),0)
 EXT2_OPTS += -r $(BR2_TARGET_ROOTFS_EXT2_RESBLKS)
 endif
 
-# Not qstrip-ing the variable, because it may contain spaces, but we must
-# qstrip it when checking. Furthermore, we need to further quote it, so
-# that the quotes do not get eaten by the echo statement when creating the
-# fakeroot script
 ifneq ($(call qstrip,$(BR2_TARGET_ROOTFS_EXT2_LABEL)),)
-EXT2_OPTS += -l '$(BR2_TARGET_ROOTFS_EXT2_LABEL)'
+EXT2_OPTS += -l $(BR2_TARGET_ROOTFS_EXT2_LABEL)
 endif
 
 ROOTFS_EXT2_DEPENDENCIES = host-mke2img