* [U-Boot] [PATCH] binman: ensure temp filenames don't collide
@ 2018-07-16 22:51 Stephen Warren
2018-07-19 1:32 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Warren @ 2018-07-16 22:51 UTC (permalink / raw)
To: u-boot
From: Stephen Warren <swarren@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@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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] binman: ensure temp filenames don't collide
2018-07-16 22:51 [U-Boot] [PATCH] binman: ensure temp filenames don't collide Stephen Warren
@ 2018-07-19 1:32 ` Simon Glass
2018-07-19 21:14 ` Stephen Warren
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2018-07-19 1:32 UTC (permalink / raw)
To: u-boot
Hi Stephen,
On 16 July 2018 at 16:51, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@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@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?
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] binman: ensure temp filenames don't collide
2018-07-19 1:32 ` Simon Glass
@ 2018-07-19 21:14 ` Stephen Warren
2018-07-20 2:17 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Warren @ 2018-07-19 21:14 UTC (permalink / raw)
To: u-boot
On 07/18/2018 07:32 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 16 July 2018 at 16:51, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@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@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?
^ permalink raw reply [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] binman: ensure temp filenames don't collide
2018-07-19 21:14 ` Stephen Warren
@ 2018-07-20 2:17 ` Simon Glass
2018-07-20 4:43 ` Stephen Warren
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2018-07-20 2:17 UTC (permalink / raw)
To: u-boot
Hi Stephen,
On 19 July 2018 at 15:14, Stephen Warren <swarren@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@wwwdotorg.org> wrote:
>>> From: Stephen Warren <swarren@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@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.
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] binman: ensure temp filenames don't collide
2018-07-20 2:17 ` Simon Glass
@ 2018-07-20 4:43 ` Stephen Warren
2018-07-21 1:52 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Warren @ 2018-07-20 4:43 UTC (permalink / raw)
To: u-boot
On 07/19/2018 08:17 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 19 July 2018 at 15:14, Stephen Warren <swarren@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@wwwdotorg.org> wrote:
>>>> From: Stephen Warren <swarren@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@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.
^ permalink raw reply [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] binman: ensure temp filenames don't collide
2018-07-20 4:43 ` Stephen Warren
@ 2018-07-21 1:52 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2018-07-21 1:52 UTC (permalink / raw)
To: u-boot
Hi Stephen,
On 19 July 2018 at 22:43, Stephen Warren <swarren@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@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@wwwdotorg.org>
> wrote:
> >>>> From: Stephen Warren <swarren@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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-21 1:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-16 22:51 [U-Boot] [PATCH] binman: ensure temp filenames don't collide Stephen Warren
2018-07-19 1:32 ` Simon Glass
2018-07-19 21:14 ` Stephen Warren
2018-07-20 2:17 ` Simon Glass
2018-07-20 4:43 ` Stephen Warren
2018-07-21 1:52 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.