1. Patchew
  2. linux
  3. [PATCH v2 0/3] arm64: Add a build target for Flat Image Tree
  4. View patch

[PATCH v2 2/3] arm: boot: Use double quotes for image name

Simon Glass posted 3 patches 2 years, 1 month ago
There is a newer version of this series
[PATCH v2 2/3] arm: boot: Use double quotes for image name
Posted by Simon Glass 2 years, 1 month ago 
The use of single quotes in the image name causes them to appear in
the image description when the uImage is created. Use double quotes, to
avoid this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Split double-quote change out into its own patch

 scripts/Makefile.lib | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 68d0134bdbf9..03e79e319293 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -487,7 +487,7 @@ UIMAGE_OPTS-y ?=
 UIMAGE_TYPE ?= kernel
 UIMAGE_LOADADDR ?= arch_must_set_this
 UIMAGE_ENTRYADDR ?= $(UIMAGE_LOADADDR)
-UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
+UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
 
 quiet_cmd_uimage = UIMAGE  $@
       cmd_uimage = $(BASH) $(MKIMAGE) -A $(UIMAGE_ARCH) -O linux \
-- 
2.42.0.869.gea05f2083d-goog
Re: [PATCH v2 2/3] arm: boot: Use double quotes for image name
Posted by Masahiro Yamada 2 years, 1 month ago 
On Sat, Nov 4, 2023 at 9:42 PM Simon Glass <sjg@chromium.org> wrote:
>
> The use of single quotes in the image name causes them to appear in
> the image description when the uImage is created. Use double quotes, to
> avoid this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2:
> - Split double-quote change out into its own patch
>
>  scripts/Makefile.lib | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 68d0134bdbf9..03e79e319293 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -487,7 +487,7 @@ UIMAGE_OPTS-y ?=
>  UIMAGE_TYPE ?= kernel
>  UIMAGE_LOADADDR ?= arch_must_set_this
>  UIMAGE_ENTRYADDR ?= $(UIMAGE_LOADADDR)
> -UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> +UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
>
>  quiet_cmd_uimage = UIMAGE  $@
>        cmd_uimage = $(BASH) $(MKIMAGE) -A $(UIMAGE_ARCH) -O linux \
> --
> 2.42.0.869.gea05f2083d-goog
>


NACK.


This is because you are doing *WRONG* in 3/3.

Look at your code closely.

https://lore.kernel.org/linux-kbuild/20231104194207.3370542-4-sjg@chromium.org/T/#me2fb68151d6f4f330808406f9a711fffee149529



In the mainline kernel, the quotation appears
only in the definition of UIMAGE_NAME.


masahiro@zoe:~/ref/linux(master)$ git grep UIMAGE_NAME
scripts/Makefile.lib:UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
scripts/Makefile.lib:                   -n $(UIMAGE_NAME) -d $< $@


The single quotes are consumed by shell.






This is mainline + your patch set.

masahiro@zoe:~/ref/linux(simon-v2)$ git grep UIMAGE_NAME
scripts/Makefile.lib:UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
scripts/Makefile.lib:                   -n "$(UIMAGE_NAME)" -d $< $@
scripts/Makefile.lib:                   --name "$(UIMAGE_NAME)" \


You quoted the definition of UIMAGE_NAME,
and also variable references.




See how it is expanded.


--name "$(UIMAGE_NAME)"


 ==>


--name ""Linux-$(KERNELRELEASE)""


 ==>


--name Linux-$(KERNELRELEASE)




You added double quotes in a row, just to cancel it.



-- 
Best Regards
Masahiro Yamada
Re: [PATCH v2 2/3] arm: boot: Use double quotes for image name
Posted by Simon Glass 2 years, 1 month ago 
Hi Masahiro,

On Tue, 7 Nov 2023 at 03:13, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Sat, Nov 4, 2023 at 9:42 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > The use of single quotes in the image name causes them to appear in
> > the image description when the uImage is created. Use double quotes, to
> > avoid this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Split double-quote change out into its own patch
> >
> >  scripts/Makefile.lib | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 68d0134bdbf9..03e79e319293 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -487,7 +487,7 @@ UIMAGE_OPTS-y ?=
> >  UIMAGE_TYPE ?= kernel
> >  UIMAGE_LOADADDR ?= arch_must_set_this
> >  UIMAGE_ENTRYADDR ?= $(UIMAGE_LOADADDR)
> > -UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> > +UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
> >
> >  quiet_cmd_uimage = UIMAGE  $@
> >        cmd_uimage = $(BASH) $(MKIMAGE) -A $(UIMAGE_ARCH) -O linux \
> > --
> > 2.42.0.869.gea05f2083d-goog
> >
>
>
> NACK.
>
>
> This is because you are doing *WRONG* in 3/3.
>
> Look at your code closely.
>
> https://lore.kernel.org/linux-kbuild/20231104194207.3370542-4-sjg@chromium.org/T/#me2fb68151d6f4f330808406f9a711fffee149529
>
>
>
> In the mainline kernel, the quotation appears
> only in the definition of UIMAGE_NAME.
>
>
> masahiro@zoe:~/ref/linux(master)$ git grep UIMAGE_NAME
> scripts/Makefile.lib:UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> scripts/Makefile.lib:                   -n $(UIMAGE_NAME) -d $< $@
>
>
> The single quotes are consumed by shell.
>
>
>
>
>
>
> This is mainline + your patch set.
>
> masahiro@zoe:~/ref/linux(simon-v2)$ git grep UIMAGE_NAME
> scripts/Makefile.lib:UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
> scripts/Makefile.lib:                   -n "$(UIMAGE_NAME)" -d $< $@
> scripts/Makefile.lib:                   --name "$(UIMAGE_NAME)" \
>
>
> You quoted the definition of UIMAGE_NAME,
> and also variable references.
>
>
>
>
> See how it is expanded.
>
>
> --name "$(UIMAGE_NAME)"
>
>
>  ==>
>
>
> --name ""Linux-$(KERNELRELEASE)""
>
>
>  ==>
>
>
> --name Linux-$(KERNELRELEASE)
>
>
>
>
> You added double quotes in a row, just to cancel it.

Yes, I understand that. But without the quotes in -n "$(UIMAGE_NAME)"
then the name cannot contain spaces. So we do need some sort of
quoting, right?

It just seems strange to use single quotes in a Makefile variable. I
found it confusing.

I think you are saying you want to keep the single quotes in the var
declaration and drop the quotes from the cmd_fit rule. I am OK with
that, but I do think it is unusual not to quote something which might
have spaces. It may cause confusion for others, as it did for me?

Anyway, I'll send a new version with the quoting reverted.

Regards,
Simon
Re: [PATCH v2 2/3] arm: boot: Use double quotes for image name
Posted by Masahiro Yamada 2 years, 1 month ago 
Hi Simon,


On Tue, Nov 7, 2023 at 3:11 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahiro,
>
> On Tue, 7 Nov 2023 at 03:13, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Sat, Nov 4, 2023 at 9:42 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > The use of single quotes in the image name causes them to appear in
> > > the image description when the uImage is created. Use double quotes, to
> > > avoid this.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - Split double-quote change out into its own patch
> > >
> > >  scripts/Makefile.lib | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > index 68d0134bdbf9..03e79e319293 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -487,7 +487,7 @@ UIMAGE_OPTS-y ?=
> > >  UIMAGE_TYPE ?= kernel
> > >  UIMAGE_LOADADDR ?= arch_must_set_this
> > >  UIMAGE_ENTRYADDR ?= $(UIMAGE_LOADADDR)
> > > -UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> > > +UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
> > >
> > >  quiet_cmd_uimage = UIMAGE  $@
> > >        cmd_uimage = $(BASH) $(MKIMAGE) -A $(UIMAGE_ARCH) -O linux \
> > > --
> > > 2.42.0.869.gea05f2083d-goog
> > >
> >
> >
> > NACK.
> >
> >
> > This is because you are doing *WRONG* in 3/3.
> >
> > Look at your code closely.
> >
> > https://lore.kernel.org/linux-kbuild/20231104194207.3370542-4-sjg@chromium.org/T/#me2fb68151d6f4f330808406f9a711fffee149529
> >
> >
> >
> > In the mainline kernel, the quotation appears
> > only in the definition of UIMAGE_NAME.
> >
> >
> > masahiro@zoe:~/ref/linux(master)$ git grep UIMAGE_NAME
> > scripts/Makefile.lib:UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> > scripts/Makefile.lib:                   -n $(UIMAGE_NAME) -d $< $@
> >
> >
> > The single quotes are consumed by shell.
> >
> >
> >
> >
> >
> >
> > This is mainline + your patch set.
> >
> > masahiro@zoe:~/ref/linux(simon-v2)$ git grep UIMAGE_NAME
> > scripts/Makefile.lib:UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
> > scripts/Makefile.lib:                   -n "$(UIMAGE_NAME)" -d $< $@
> > scripts/Makefile.lib:                   --name "$(UIMAGE_NAME)" \
> >
> >
> > You quoted the definition of UIMAGE_NAME,
> > and also variable references.
> >
> >
> >
> >
> > See how it is expanded.
> >
> >
> > --name "$(UIMAGE_NAME)"
> >
> >
> >  ==>
> >
> >
> > --name ""Linux-$(KERNELRELEASE)""
> >
> >
> >  ==>
> >
> >
> > --name Linux-$(KERNELRELEASE)
> >
> >
> >
> >
> > You added double quotes in a row, just to cancel it.
>
> Yes, I understand that. But without the quotes in -n "$(UIMAGE_NAME)"
> then the name cannot contain spaces. So we do need some sort of
> quoting, right?


Yes.

If you move the quoting to the variable reference,
it is acceptable because there is a good reason to do so.



UIMAGE_NAME ?= Linux-$(KERNELRELEASE)


            ...
             -n '$(UIMAGE_NAME)' -d $< $@


This is the correct change.



>
> It just seems strange to use single quotes in a Makefile variable. I
> found it confusing.


Right. Why don't you remove it, then?


For clarification, there is no concept of quoting in GNU Make.

The single quote character ' and the double quote character " are
just normal characters for Make.

GNU Make handles them just like alphabets and numbers.

GNU Make just replaces $(UIMAGE_NAME)
with 'Linux-$(KERNELRELEASE)' verbatim.


It is the _shell_ that understands the quoting.

Just in case here is the spec for
"2.2.2 Single-Quotes" vs "2.2.3 Double-Quotes"

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html


Shell supports both single-quoting and double-quoting
for good reasons.

There is no good or bad because both of them are meaningful.






>
> I think you are saying you want to keep the single quotes in the var
> declaration and drop the quotes from the cmd_fit rule. I am OK with
> that, but I do think it is unusual not to quote something which might
> have spaces. It may cause confusion for others, as it did for me?
>
> Anyway, I'll send a new version with the quoting reverted.
>


Please move the single quotes as I suggested above.

The reason is because UIMAGE_NAME can be passed-in
by a user and it can contain whitespaces.





> Regards,
> Simon



-- 
Best Regards
Masahiro Yamada
Re: [PATCH v2 2/3] arm: boot: Use double quotes for image name
Posted by Simon Glass 2 years, 1 month ago 
Hi Masahiro,

On Tue, 7 Nov 2023 at 07:13, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Hi Simon,
>
>
> On Tue, Nov 7, 2023 at 3:11 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Masahiro,
> >
> > On Tue, 7 Nov 2023 at 03:13, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > On Sat, Nov 4, 2023 at 9:42 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > The use of single quotes in the image name causes them to appear in
> > > > the image description when the uImage is created. Use double quotes, to
> > > > avoid this.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Split double-quote change out into its own patch
> > > >
> > > >  scripts/Makefile.lib | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > index 68d0134bdbf9..03e79e319293 100644
> > > > --- a/scripts/Makefile.lib
> > > > +++ b/scripts/Makefile.lib
> > > > @@ -487,7 +487,7 @@ UIMAGE_OPTS-y ?=
> > > >  UIMAGE_TYPE ?= kernel
> > > >  UIMAGE_LOADADDR ?= arch_must_set_this
> > > >  UIMAGE_ENTRYADDR ?= $(UIMAGE_LOADADDR)
> > > > -UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> > > > +UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
> > > >
> > > >  quiet_cmd_uimage = UIMAGE  $@
> > > >        cmd_uimage = $(BASH) $(MKIMAGE) -A $(UIMAGE_ARCH) -O linux \
> > > > --
> > > > 2.42.0.869.gea05f2083d-goog
> > > >
> > >
> > >
> > > NACK.
> > >
> > >
> > > This is because you are doing *WRONG* in 3/3.
> > >
> > > Look at your code closely.
> > >
> > > https://lore.kernel.org/linux-kbuild/20231104194207.3370542-4-sjg@chromium.org/T/#me2fb68151d6f4f330808406f9a711fffee149529
> > >
> > >
> > >
> > > In the mainline kernel, the quotation appears
> > > only in the definition of UIMAGE_NAME.
> > >
> > >
> > > masahiro@zoe:~/ref/linux(master)$ git grep UIMAGE_NAME
> > > scripts/Makefile.lib:UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> > > scripts/Makefile.lib:                   -n $(UIMAGE_NAME) -d $< $@
> > >
> > >
> > > The single quotes are consumed by shell.
> > >
> > >
> > >
> > >
> > >
> > >
> > > This is mainline + your patch set.
> > >
> > > masahiro@zoe:~/ref/linux(simon-v2)$ git grep UIMAGE_NAME
> > > scripts/Makefile.lib:UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
> > > scripts/Makefile.lib:                   -n "$(UIMAGE_NAME)" -d $< $@
> > > scripts/Makefile.lib:                   --name "$(UIMAGE_NAME)" \
> > >
> > >
> > > You quoted the definition of UIMAGE_NAME,
> > > and also variable references.
> > >
> > >
> > >
> > >
> > > See how it is expanded.
> > >
> > >
> > > --name "$(UIMAGE_NAME)"
> > >
> > >
> > >  ==>
> > >
> > >
> > > --name ""Linux-$(KERNELRELEASE)""
> > >
> > >
> > >  ==>
> > >
> > >
> > > --name Linux-$(KERNELRELEASE)
> > >
> > >
> > >
> > >
> > > You added double quotes in a row, just to cancel it.
> >
> > Yes, I understand that. But without the quotes in -n "$(UIMAGE_NAME)"
> > then the name cannot contain spaces. So we do need some sort of
> > quoting, right?
>
>
> Yes.
>
> If you move the quoting to the variable reference,
> it is acceptable because there is a good reason to do so.
>
>
>
> UIMAGE_NAME ?= Linux-$(KERNELRELEASE)
>
>
>             ...
>              -n '$(UIMAGE_NAME)' -d $< $@
>
>
> This is the correct change.

OK.

>
>
>
> >
> > It just seems strange to use single quotes in a Makefile variable. I
> > found it confusing.
>
>
> Right. Why don't you remove it, then?
>
>
> For clarification, there is no concept of quoting in GNU Make.
>
> The single quote character ' and the double quote character " are
> just normal characters for Make.
>
> GNU Make handles them just like alphabets and numbers.
>
> GNU Make just replaces $(UIMAGE_NAME)
> with 'Linux-$(KERNELRELEASE)' verbatim.
>
>
> It is the _shell_ that understands the quoting.
>
> Just in case here is the spec for
> "2.2.2 Single-Quotes" vs "2.2.3 Double-Quotes"
>
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
>
>
> Shell supports both single-quoting and double-quoting
> for good reasons.
>
> There is no good or bad because both of them are meaningful.

Yes...I suppose I knew that Makefiles are completely literal, but
thanks for the pointers. I tend to use double quotes by default and
single quotes only when I have to...but it doesn't really matter so
long as it is consistent.

Anyway, moving the single quotes away from the var removes the
confusion I had at the start of all of this.

>
>
>
>
>
>
> >
> > I think you are saying you want to keep the single quotes in the var
> > declaration and drop the quotes from the cmd_fit rule. I am OK with
> > that, but I do think it is unusual not to quote something which might
> > have spaces. It may cause confusion for others, as it did for me?
> >
> > Anyway, I'll send a new version with the quoting reverted.
> >
>
>
> Please move the single quotes as I suggested above.
>
> The reason is because UIMAGE_NAME can be passed-in
> by a user and it can contain whitespaces.

OK, done in v4.

Regards,
Simon

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.