Laszlo Ersek
2023-Apr-19 13:53 UTC
[Libguestfs] [libnbd PATCH 16/18] tests/requires: wrap source code at 80 characters
On 4/19/23 15:37, Eric Blake wrote:> On Tue, Apr 18, 2023 at 07:26:29PM +0200, Laszlo Ersek wrote: >> Embedding a shell script in a multi-line C string literal is an exercise >> in pain. I can see why the original author (whom I shall not look up with >> git-blame :) ) went for the easy route. Still, we want the source code to >> fit in 80 columns. >> >> Note: in my "interop/test-suite.log", I see >> >>> SKIP: interop-qemu-nbd-tls-certs >>> ===============================>>> >>> requires test -d /home/lacos/src/v2v/libnbd/tests/pki >>> Test skipped because prerequisite is missing or not working. >>> SKIP interop-qemu-nbd-tls-certs (exit status: 77) >>> >>> SKIP: interop-qemu-nbd-tls-psk >>> =============================>>> >>> requires test -f /home/lacos/src/v2v/libnbd/tests/keys.psk >>> Test skipped because prerequisite is missing or not working. >>> SKIP interop-qemu-nbd-tls-psk (exit status: 77) >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2172516 >> Signed-off-by: Laszlo Ersek <lersek at redhat.com> >> --- >> tests/requires.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/tests/requires.c b/tests/requires.c >> index 199e30605473..cc5fd77b9f27 100644 >> --- a/tests/requires.c >> +++ b/tests/requires.c >> @@ -57,7 +57,13 @@ requires_qemu_nbd_tls_support (const char *qemu_nbd) >> * interested in the error message that it prints. >> */ >> snprintf (cmd, sizeof cmd, >> - "if %s --object tls-creds-x509,id=tls0 |& grep -sq 'TLS credentials support requires GNUTLS'; then exit 1; else exit 0; fi", >> + "if %s --object tls-creds-x509,id=tls0 \\\n" >> + " |& grep -sq 'TLS credentials support requires GNUTLS'\n" > > Note: |& is a bashism; but system() might not invoke bash. So this is > already non-portable code; and we should be fixing that. (I don't > mind doing it as a followup if you end up pushing before seeing this > message).I noticed |& too; my thought was "TIL". :)> >> + "then\n" >> + " exit 1\n" >> + "else\n" >> + " exit 0\n" >> + "fi\n", > > For that matter, 'if ...; then exit 1; else exit 0; fi' can be > compressed to '! ...'. >Yes, I noticed that too, but I assumed there was a finer point I was missing. :/ I'll redo this patch then. Thanks! Laszlo
Richard W.M. Jones
2023-Apr-19 14:32 UTC
[Libguestfs] [libnbd PATCH 16/18] tests/requires: wrap source code at 80 characters
On Wed, Apr 19, 2023 at 03:53:57PM +0200, Laszlo Ersek wrote:> On 4/19/23 15:37, Eric Blake wrote: > > On Tue, Apr 18, 2023 at 07:26:29PM +0200, Laszlo Ersek wrote: > >> Embedding a shell script in a multi-line C string literal is an exercise > >> in pain. I can see why the original author (whom I shall not look up with > >> git-blame :) ) went for the easy route. Still, we want the source code to > >> fit in 80 columns. > >> > >> Note: in my "interop/test-suite.log", I see > >> > >>> SKIP: interop-qemu-nbd-tls-certs > >>> ===============================> >>> > >>> requires test -d /home/lacos/src/v2v/libnbd/tests/pki > >>> Test skipped because prerequisite is missing or not working. > >>> SKIP interop-qemu-nbd-tls-certs (exit status: 77) > >>> > >>> SKIP: interop-qemu-nbd-tls-psk > >>> =============================> >>> > >>> requires test -f /home/lacos/src/v2v/libnbd/tests/keys.psk > >>> Test skipped because prerequisite is missing or not working. > >>> SKIP interop-qemu-nbd-tls-psk (exit status: 77) > >> > >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2172516 > >> Signed-off-by: Laszlo Ersek <lersek at redhat.com> > >> --- > >> tests/requires.c | 16 ++++++++++++++-- > >> 1 file changed, 14 insertions(+), 2 deletions(-) > >> > >> diff --git a/tests/requires.c b/tests/requires.c > >> index 199e30605473..cc5fd77b9f27 100644 > >> --- a/tests/requires.c > >> +++ b/tests/requires.c > >> @@ -57,7 +57,13 @@ requires_qemu_nbd_tls_support (const char *qemu_nbd) > >> * interested in the error message that it prints. > >> */ > >> snprintf (cmd, sizeof cmd, > >> - "if %s --object tls-creds-x509,id=tls0 |& grep -sq 'TLS credentials support requires GNUTLS'; then exit 1; else exit 0; fi", > >> + "if %s --object tls-creds-x509,id=tls0 \\\n" > >> + " |& grep -sq 'TLS credentials support requires GNUTLS'\n" > > > > Note: |& is a bashism; but system() might not invoke bash. So this is > > already non-portable code; and we should be fixing that. (I don't > > mind doing it as a followup if you end up pushing before seeing this > > message). > > I noticed |& too; my thought was "TIL". :) > > > > >> + "then\n" > >> + " exit 1\n" > >> + "else\n" > >> + " exit 0\n" > >> + "fi\n", > > > > For that matter, 'if ...; then exit 1; else exit 0; fi' can be > > compressed to '! ...'. > > > > Yes, I noticed that too, but I assumed there was a finer point I was > missing. :/I don't think there was anything deep here. If '!...' is the same then let's use that. Rich.> I'll redo this patch then. > > Thanks! > Laszlo > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Laszlo Ersek
2023-Apr-19 15:27 UTC
[Libguestfs] [libnbd PATCH 16/18] tests/requires: wrap source code at 80 characters
On 4/19/23 15:53, Laszlo Ersek wrote:> On 4/19/23 15:37, Eric Blake wrote: >> On Tue, Apr 18, 2023 at 07:26:29PM +0200, Laszlo Ersek wrote: >>> Embedding a shell script in a multi-line C string literal is an exercise >>> in pain. I can see why the original author (whom I shall not look up with >>> git-blame :) ) went for the easy route. Still, we want the source code to >>> fit in 80 columns. >>> >>> Note: in my "interop/test-suite.log", I see >>> >>>> SKIP: interop-qemu-nbd-tls-certs >>>> ===============================>>>> >>>> requires test -d /home/lacos/src/v2v/libnbd/tests/pki >>>> Test skipped because prerequisite is missing or not working. >>>> SKIP interop-qemu-nbd-tls-certs (exit status: 77) >>>> >>>> SKIP: interop-qemu-nbd-tls-psk >>>> =============================>>>> >>>> requires test -f /home/lacos/src/v2v/libnbd/tests/keys.psk >>>> Test skipped because prerequisite is missing or not working. >>>> SKIP interop-qemu-nbd-tls-psk (exit status: 77) >>> >>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2172516 >>> Signed-off-by: Laszlo Ersek <lersek at redhat.com> >>> --- >>> tests/requires.c | 16 ++++++++++++++-- >>> 1 file changed, 14 insertions(+), 2 deletions(-) >>> >>> diff --git a/tests/requires.c b/tests/requires.c >>> index 199e30605473..cc5fd77b9f27 100644 >>> --- a/tests/requires.c >>> +++ b/tests/requires.c >>> @@ -57,7 +57,13 @@ requires_qemu_nbd_tls_support (const char *qemu_nbd) >>> * interested in the error message that it prints. >>> */ >>> snprintf (cmd, sizeof cmd, >>> - "if %s --object tls-creds-x509,id=tls0 |& grep -sq 'TLS credentials support requires GNUTLS'; then exit 1; else exit 0; fi", >>> + "if %s --object tls-creds-x509,id=tls0 \\\n" >>> + " |& grep -sq 'TLS credentials support requires GNUTLS'\n" >> >> Note: |& is a bashism; but system() might not invoke bash. So this is >> already non-portable code; and we should be fixing that. (I don't >> mind doing it as a followup if you end up pushing before seeing this >> message). > > I noticed |& too; my thought was "TIL". :) > >> >>> + "then\n" >>> + " exit 1\n" >>> + "else\n" >>> + " exit 0\n" >>> + "fi\n", >> >> For that matter, 'if ...; then exit 1; else exit 0; fi' can be >> compressed to '! ...'. >> > > Yes, I noticed that too, but I assumed there was a finer point I was > missing. :/Ah, it's not too relevant, but now I remember what was bothering me, about "!": if you embed the original construct in a "set -e" environment, it will exit with status 1 on the appropriate branch; but if you embed the "! ..." construct, it won't exit! Anyway, the context is not like that here. We pass the command string to system(), and I understand "set -e" is not in effect there. Laszlo
Eric Blake
2023-Apr-19 18:27 UTC
[Libguestfs] [libnbd PATCH 16/18] tests/requires: wrap source code at 80 characters
On Wed, Apr 19, 2023 at 03:53:57PM +0200, Laszlo Ersek wrote:> >> snprintf (cmd, sizeof cmd, > >> - "if %s --object tls-creds-x509,id=tls0 |& grep -sq 'TLS credentials support requires GNUTLS'; then exit 1; else exit 0; fi", > >> + "if %s --object tls-creds-x509,id=tls0 \\\n" > >> + " |& grep -sq 'TLS credentials support requires GNUTLS'\n" > > > > Note: |& is a bashism; but system() might not invoke bash. So this is > > already non-portable code; and we should be fixing that. (I don't > > mind doing it as a followup if you end up pushing before seeing this > > message). > > I noticed |& too; my thought was "TIL". :) > > > > >> + "then\n" > >> + " exit 1\n" > >> + "else\n" > >> + " exit 0\n" > >> + "fi\n", > > > > For that matter, 'if ...; then exit 1; else exit 0; fi' can be > > compressed to '! ...'. > > > > Yes, I noticed that too, but I assumed there was a finer point I was > missing. :/autoconf still avoids it because of portability to Solaris /bin/sh (not /bin/xpg4/sh), which lacked !. But that's ancient history these days; ALL modern sh support ! correctly. (Note that support for '! command...' is different than 'test ! expr...', which itself has its own set of portability pitfalls that autoconf cares about; but that is yet another place where modern sh is now quite portable). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org