[U-Boot] [PATCH] binman: ensure temp filenames don't collide

[Simon Glass]

Hi Stephen,

On 19 July 2018 at 22:43, Stephen Warren <swarren at wwwdotorg.org> wrote:

> On 07/19/2018 08:17 PM, Simon Glass wrote:
> > Hi Stephen,
> >
> > On 19 July 2018 at 15:14, Stephen Warren <swarren at wwwdotorg.org> wrote:
> >> On 07/18/2018 07:32 PM, Simon Glass wrote:
> >>> Hi Stephen,
> >>>
> >>> On 16 July 2018 at 16:51, Stephen Warren <swarren at wwwdotorg.org>
> wrote:
> >>>> From: Stephen Warren <swarren at nvidia.com>
> >>>>
> >>>> The U-Boot Makefile can invoke binman multiple times in parallel. This
> >>>> is problematic because binman uses a static hard-coded temporary file
> >>>> name. If two instances of binman use that filename at the same time,
> one
> >>>> writing one reading, they may silently read the wrong content or
> actively
> >>>> detect missing signatures and error out the build process. Fix this by
> >>>> using a PID-specific filename instead.
> >>>>
> >>>> Signed-off-by: Stephen Warren <swarren at nvidia.com>
> >>>> ---
> >>>>  tools/binman/control.py | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/tools/binman/control.py b/tools/binman/control.py
> >>>> index a40b300fdacb..515999278949 100644
> >>>> --- a/tools/binman/control.py
> >>>> +++ b/tools/binman/control.py
> >>>> @@ -121,7 +121,7 @@ def Binman(options, args):
> >>>>              # output into a file in our output directly. Then scan
> it for use
> >>>>              # in binman.
> >>>>              dtb_fname = fdt_util.EnsureCompiled(dtb_fname)
> >>>> -            fname = tools.GetOutputFilename('u-boot-out.dtb')
> >>>> +            fname = tools.GetOutputFilename('u-boot-out.dtb') +
> str(os.getpid())
> >>>>              with open(dtb_fname) as infd:
> >>>>                  with open(fname, 'wb') as outfd:
> >>>>                      outfd.write(infd.read())
> >>>> --
> >>>> 2.18.0
> >>>>
> >>>
> >>> But the output directory is itself (normally) a temporary dir. That
> >>> determines the directly which GetOutputFilename() uses. So I am not
> >>> sure how this can happen in practice?
> >>
> >> IIRC, the output directory for all 3 files was the same; the root of the
> >> build output directory. Perhaps the fact I run "make O=build-xxx" rather
> >> than just "make" makes a difference?
> >
> > Yes you are right - the -O flag sets the output directory and it
> > no-longer uses a temporary directory.
> >
> > But if we add a PID to every filename then we end up with multiple
> > output files and we don't know which is right.
> >
> > I think the correct fix is to change the Makefile to only run binman
> > once. It makes no sense to run it multiple times.
>
> IIRC, this filename is just a temp file that eventually gets removed
> after binman runs. Certainly there are not *.${pid} files left around
> after the build has completed.
>

This is what I see with your patch:

u-boot-out.dtb198273
u-boot-out.dtb198277
u-boot-out.dtb198280

I think there are two options:

1. Add an arg to binman to indicate which output image to create, so that
binman runs three times but only one of those generates the .dtb output
file.

2. Use a GNU make pattern rule, as described here:
https://www.gnu.org/software/make/manual/html_node/Pattern-Examples.html#Pattern-Examples

I'll send a patch for the latter which seems easier to me. But I will
probably have to update binman to support (1) too.

Regards,
Simon