[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK7LNARzfMDBX9mhCaCV8QxpTic6A3JYOPBManJNh9Jas=-T9A@mail.gmail.com>
Date: Tue, 7 Nov 2023 16:12:30 +0200
From: Masahiro Yamada <masahiroy@...nel.org>
To: Simon Glass <sjg@...omium.org>
Cc: linux-arm-kernel@...ts.infradead.org,
U-Boot Mailing List <u-boot@...ts.denx.de>,
Tom Rini <trini@...sulko.com>,
Nathan Chancellor <nathan@...nel.org>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Nicolas Schier <nicolas@...sle.eu>,
linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] arm: boot: Use double quotes for image name
Hi Simon,
On Tue, Nov 7, 2023 at 3:11 PM Simon Glass <sjg@...omium.org> wrote:
>
> Hi Masahiro,
>
> On Tue, 7 Nov 2023 at 03:13, Masahiro Yamada <masahiroy@...nel.org> wrote:
> >
> > On Sat, Nov 4, 2023 at 9:42 PM Simon Glass <sjg@...omium.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@...omium.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
Powered by blists - more mailing lists