| Message ID | 20230511160116.398793-1-peron.clem@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] runfvp: switch working directory to allow path relative to fvpconf | expand |
Hi Clement, On 11/05/2023 17:01, Clément Péron via lists.yoctoproject.org wrote: > fvpconf file contains absolute path that leak builder path. > > Allow to add relative path by changing the chdir before loading > the config file. The fvpconf file is read by both runfvp and also a custom OE testimage controller [1] which we use heavily for runtime validation - I think these patches will break this use case. It should be possible to set the 'cwd' argument of subprocess.Popen in the common code [2]. It might require another patch to move conffile loading into the common code though so that its filename is accessible... We also have an oe-selftest for this common code [3], which may need updating. [1] https://git.yoctoproject.org/meta-arm/tree/meta-arm/lib/oeqa/controllers/fvp.py [2] https://git.yoctoproject.org/meta-arm/tree/meta-arm/lib/fvp/runner.py#n99 [3] https://git.yoctoproject.org/meta-arm/tree/meta-arm/lib/oeqa/selftest/cases/runfvp.py Peter > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > scripts/runfvp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/scripts/runfvp b/scripts/runfvp > index e4b00abc..f0d821be 100755 > --- a/scripts/runfvp > +++ b/scripts/runfvp > @@ -87,6 +87,12 @@ def runfvp(cli_args): > config_file = args.config > else: > config_file = conffile.find(args.config) > + > + # Path inside the config could be relative to the config file > + working_directory = os.path.dirname(config_file) > + logger.debug(f"Switching the working directory to {working_directory}") > + os.chdir(working_directory) > + > logger.debug(f"Loading {config_file}") > config = conffile.load(config_file) > start_fvp(args, config, extra_args) > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#4641): https://lists.yoctoproject.org/g/meta-arm/message/4641 > Mute This Topic: https://lists.yoctoproject.org/mt/98830760/5715260 > Group Owner: meta-arm+owner@lists.yoctoproject.org > Unsubscribe: https://lists.yoctoproject.org/g/meta-arm/unsub [peter.hoyes@arm.com] > -=-=-=-=-=-=-=-=-=-=-=- >
Hi Peter, On Fri, 12 May 2023 at 16:19, Peter Hoyes <Peter.Hoyes@arm.com> wrote: > > Hi Clement, > > On 11/05/2023 17:01, Clément Péron via lists.yoctoproject.org wrote: > > fvpconf file contains absolute path that leak builder path. > > > > Allow to add relative path by changing the chdir before loading > > the config file. > > The fvpconf file is read by both runfvp and also a custom OE testimage > controller [1] which we use heavily for runtime validation - I think > these patches will break this use case. Thanks for the review and the explanation, So this will make the config read multiple times right? one time in the caller and one time in the callee. What about passing the conf working directory as a param of FVPRunner.start() ? Something like this: def start(self, fvpconf, extra_args=[], terminal_choice="none", stdout=subprocess.PIPE, fvp_cwd=None): Regards, Clement > > It should be possible to set the 'cwd' argument of subprocess.Popen in > the common code [2]. It might require another patch to move conffile > loading into the common code though so that its filename is accessible... > > We also have an oe-selftest for this common code [3], which may need > updating. > > [1] > https://git.yoctoproject.org/meta-arm/tree/meta-arm/lib/oeqa/controllers/fvp.py > [2] > https://git.yoctoproject.org/meta-arm/tree/meta-arm/lib/fvp/runner.py#n99 > [3] > https://git.yoctoproject.org/meta-arm/tree/meta-arm/lib/oeqa/selftest/cases/runfvp.py > > Peter > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > scripts/runfvp | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/scripts/runfvp b/scripts/runfvp > > index e4b00abc..f0d821be 100755 > > --- a/scripts/runfvp > > +++ b/scripts/runfvp > > @@ -87,6 +87,12 @@ def runfvp(cli_args): > > config_file = args.config > > else: > > config_file = conffile.find(args.config) > > + > > + # Path inside the config could be relative to the config file > > + working_directory = os.path.dirname(config_file) > > + logger.debug(f"Switching the working directory to {working_directory}") > > + os.chdir(working_directory) > > + > > logger.debug(f"Loading {config_file}") > > config = conffile.load(config_file) > > start_fvp(args, config, extra_args) > > > > -=-=-=-=-=-=-=-=-=-=-=- > > Links: You receive all messages sent to this group. > > View/Reply Online (#4641): https://lists.yoctoproject.org/g/meta-arm/message/4641 > > Mute This Topic: https://lists.yoctoproject.org/mt/98830760/5715260 > > Group Owner: meta-arm+owner@lists.yoctoproject.org > > Unsubscribe: https://lists.yoctoproject.org/g/meta-arm/unsub [peter.hoyes@arm.com] > > -=-=-=-=-=-=-=-=-=-=-=- > >
On 15/05/2023 10:27, Clément Péron wrote: > Hi Peter, > > On Fri, 12 May 2023 at 16:19, Peter Hoyes <Peter.Hoyes@arm.com> wrote: >> Hi Clement, >> >> On 11/05/2023 17:01, Clément Péron via lists.yoctoproject.org wrote: >>> fvpconf file contains absolute path that leak builder path. >>> >>> Allow to add relative path by changing the chdir before loading >>> the config file. >> The fvpconf file is read by both runfvp and also a custom OE testimage >> controller [1] which we use heavily for runtime validation - I think >> these patches will break this use case. > Thanks for the review and the explanation, Thanks for pursuing this refactor. > > So this will make the config read multiple times right? one time in > the caller and one time in the callee. I'm not sure I understand fully, but the suggestion is to move all config reading logic inside FVPRunner.start() so that we only call conffile.load() in one place and the number of arguments to FVPRunner.start() is reduced. > > What about passing the conf working directory as a param of FVPRunner.start() ? > > Something like this: > def start(self, fvpconf, extra_args=[], terminal_choice="none", > stdout=subprocess.PIPE, fvp_cwd=None): I think it would make more sense to derive the cwd from the fvpconf filename? If the fvpconf file contents are inherently dependent on the cwd I don't think it makes sense to support alternate cwds. Peter > > Regards, > Clement > > >> It should be possible to set the 'cwd' argument of subprocess.Popen in >> the common code [2]. It might require another patch to move conffile >> loading into the common code though so that its filename is accessible... >> >> We also have an oe-selftest for this common code [3], which may need >> updating. >> >> [1] >> https://git.yoctoproject.org/meta-arm/tree/meta-arm/lib/oeqa/controllers/fvp.py >> [2] >> https://git.yoctoproject.org/meta-arm/tree/meta-arm/lib/fvp/runner.py#n99 >> [3] >> https://git.yoctoproject.org/meta-arm/tree/meta-arm/lib/oeqa/selftest/cases/runfvp.py >> >> Peter >> >>> Signed-off-by: Clément Péron <peron.clem@gmail.com> >>> --- >>> scripts/runfvp | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/scripts/runfvp b/scripts/runfvp >>> index e4b00abc..f0d821be 100755 >>> --- a/scripts/runfvp >>> +++ b/scripts/runfvp >>> @@ -87,6 +87,12 @@ def runfvp(cli_args): >>> config_file = args.config >>> else: >>> config_file = conffile.find(args.config) >>> + >>> + # Path inside the config could be relative to the config file >>> + working_directory = os.path.dirname(config_file) >>> + logger.debug(f"Switching the working directory to {working_directory}") >>> + os.chdir(working_directory) >>> + >>> logger.debug(f"Loading {config_file}") >>> config = conffile.load(config_file) >>> start_fvp(args, config, extra_args) >>> >>> -=-=-=-=-=-=-=-=-=-=-=- >>> Links: You receive all messages sent to this group. >>> View/Reply Online (#4641): https://lists.yoctoproject.org/g/meta-arm/message/4641 >>> Mute This Topic: https://lists.yoctoproject.org/mt/98830760/5715260 >>> Group Owner: meta-arm+owner@lists.yoctoproject.org >>> Unsubscribe: https://lists.yoctoproject.org/g/meta-arm/unsub [peter.hoyes@arm.com] >>> -=-=-=-=-=-=-=-=-=-=-=- >>>
diff --git a/scripts/runfvp b/scripts/runfvp index e4b00abc..f0d821be 100755 --- a/scripts/runfvp +++ b/scripts/runfvp @@ -87,6 +87,12 @@ def runfvp(cli_args): config_file = args.config else: config_file = conffile.find(args.config) + + # Path inside the config could be relative to the config file + working_directory = os.path.dirname(config_file) + logger.debug(f"Switching the working directory to {working_directory}") + os.chdir(working_directory) + logger.debug(f"Loading {config_file}") config = conffile.load(config_file) start_fvp(args, config, extra_args)
fvpconf file contains absolute path that leak builder path. Allow to add relative path by changing the chdir before loading the config file. Signed-off-by: Clément Péron <peron.clem@gmail.com> --- scripts/runfvp | 6 ++++++ 1 file changed, 6 insertions(+)