Re: [PATCH v3 2/4] tests: Do not occlude subshell error codes

[Glenn Washburn]

On Tue, 12 Oct 2021 21:40:45 +0200
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Mon, Oct 11, 2021 at 01:06:17PM -0500, Glenn Washburn wrote:
> > On Mon, 11 Oct 2021 16:20:46 +0200
> > Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >
> > > On Thu, Oct 07, 2021 at 03:33:25PM -0500, Glenn Washburn wrote:
> > > > When a subshell's output is used as input to a "simple command", its 
> > > > return
> 
> I think you should move "(usually the test command)" from below here
> and/or add definition of "simple command".

I didn't state it, but "simple command" is defined in the man page for
dash and bash.

> 
> > > > code is not checked. These subshells contain an execution of the 
> > > > grub-shell
> > > > script which does the work of the actual test. If grub-shell returns an
> >
> > This "grub-shell" should be "subshell"
> >
> > > > error code, the test should fail. So refactor to not have the subshell 
> > > > which
> > > > contains grub-shell be direct input into a simple command (usually the 
> > > > test
> >
> > This "grub-shell" is correct, as far as I can tell.
> >
> > > > command). Mostly this is accomplished by having the output first go to a
> > > > shell variable, and then using the shell variable in the simple command.
> > > >
> > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > ---
> > > >  tests/ahci_test.in          | 7 ++++++-
> > > >  tests/cdboot_test.in        | 3 ++-
> > > >  tests/core_compress_test.in | 6 ++++--
> > > >  tests/ehci_test.in          | 7 ++++++-
> > > >  tests/fddboot_test.in       | 3 ++-
> > > >  tests/gzcompress_test.in    | 3 ++-
> > > >  tests/hddboot_test.in       | 3 ++-
> > > >  tests/lzocompress_test.in   | 3 ++-
> > > >  tests/netboot_test.in       | 3 ++-
> > > >  tests/ohci_test.in          | 7 ++++++-
> > > >  tests/pata_test.in          | 3 ++-
> > > >  tests/uhci_test.in          | 7 ++++++-
> > > >  tests/xzcompress_test.in    | 3 ++-
> > > >  13 files changed, 44 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/tests/ahci_test.in b/tests/ahci_test.in
> > > > index 9b1d85df4..1e4e3e443 100644
> > > > --- a/tests/ahci_test.in
> > > > +++ b/tests/ahci_test.in
> > > > @@ -41,7 +41,12 @@ echo "hello" > "$outfile"
> > > >
> > > >  tar cf "$imgfile" "$outfile"
> > > >
> > > > -if [ "$(echo "nativedisk; source '(ahci0)/$outfile';" | "${grubshell}" 
> > > > --qemu-opts="-drive id=disk,file=$imgfile,if=none -device ahci,id=ahci 
> > > > -device ide-hd,drive=disk,bus=ahci.0 " | tail -n 1)" != "Hello World" 
> > > > ]; then
> > > > +v=$(echo "nativedisk; source '(ahci0)/$outfile';" |
> > > > +    "${grubshell}" --qemu-opts="-drive id=disk,file=$imgfile,if=none
> > > > +                               -device ahci,id=ahci
> > > > +                               -device ide-hd,drive=disk,bus=ahci.0" |
> > > > +    tail -n 1)
> > >
> > > The first change partially contradicts what you said in the commit
> > > message. I had to take a look at the patch #3 to understand why you did
> > > not drop "tail -n 1". I think the commit message should be clarified in
> > > a such way which does not require referring to another patch to
> > > understand this one.
> >
> > Yep, mix up. Please see correction above to see if it removes the
> > contradiction you're seeing. The intent was not to refer to a further
> > patch.
> 
> I think it improves situation a bit. If you could think how to reword
> this commit message in general that (probably) would be better.

Yes, I agree. Now I'm seeing I was being more generic than I needed to
be and wasn't quite correct.

> 
> > > > +if [ "$v" != "Hello World" ]; then
> > > >     rm "$imgfile"
> > > >     rm "$outfile"
> > > >     exit 1
> > > > diff --git a/tests/cdboot_test.in b/tests/cdboot_test.in
> > > > index 75acdfedb..7229f79fb 100644
> > > > --- a/tests/cdboot_test.in
> > > > +++ b/tests/cdboot_test.in
> > > > @@ -34,6 +34,7 @@ case 
> > > > "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
> > > >         exit 0;;
> > > >  esac
> > > >
> > > > -if [ "$(echo hello | "${grubshell}" --boot=cd)" != "Hello World" ]; 
> > > > then
> > > > +v=`echo hello | "${grubshell}" --boot=cd`
> > >
> > > I would prefer if you are more consistent and use "$(...)" instead of 
> > > "`...`".
> > > The former looks more common in these scripts.
> >
> > Yep, I agree and prefer in general the former style. Not sure, why I
> > changed style there. I see its done in a few places. Want me to just
> > resend this patch or the whole series?

Now I see this change from $(...) to `...` was unintentional and due to
starting from an older commit.

> 
> I think it would be easier for me if you could merge v3 with v2 patch
> series and repost as one batch.

Ok, I'll do that.

Glenn