10:31 a.m.
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.
At Eric's suggestion, also:
- replace the non-portable control operator "|&" with the portable
redirection operator "2>&1" and the portable control operator
"|";
- replace the "if ...; then exit 1; else exit 0; fi" constructs with "!
...".
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(a)redhat.com>
---
Notes:
v2:
- v1: https://listman.redhat.com/archives/libguestfs/2023-April/031298.html
- incorporate Eric's recommendations (commit message, code)
tests/requires.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/requires.c b/tests/requires.c
index 199e30605473..2b7031532fe2 100644
--- a/tests/requires.c
+++ b/tests/requires.c
@@ -57,7 +57,8 @@ 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",
+ "! %s --object tls-creds-x509,id=tls0 2>&1 \\\n"
+ " | grep -sq 'TLS credentials support requires
GNUTLS'\n",
qemu_nbd);
requires (cmd);
}
@@ -72,7 +73,8 @@ requires_qemu_nbd_tls_psk_support (const char *qemu_nbd)
* interested in the error message that it prints.
*/
snprintf (cmd, sizeof cmd,
- "if %s --object tls-creds-psk,id=tls0 / |& grep -sq 'invalid
object type'; then exit 1; else exit 0; fi",
+ "! %s --object tls-creds-psk,id=tls0 / 2>&1 \\\n"
+ " | grep -sq 'invalid object type'\n",
qemu_nbd);
requires (cmd);
}
base-commit: a458f0644de0fa541b055299fe6a58e1f42d0073
1:21 p.m.
On Wed, Apr 19, 2023 at 05:31:27PM +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.
At Eric's suggestion, also:
- replace the non-portable control operator "|&" with the portable
redirection operator "2>&1" and the portable control operator
"|";
- replace the "if ...; then exit 1; else exit 0; fi" constructs with "!
...".
Reviewed-by: Eric Blake <eblake(a)redhat.com>
Notes:
v2:
- v1: https://listman.redhat.com/archives/libguestfs/2023-April/031298.html
- incorporate Eric's recommendations (commit message, code)
tests/requires.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/requires.c b/tests/requires.c
index 199e30605473..2b7031532fe2 100644
--- a/tests/requires.c
+++ b/tests/requires.c
@@ -57,7 +57,8 @@ 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",
+ "! %s --object tls-creds-x509,id=tls0 2>&1 \\\n"
+ " | grep -sq 'TLS credentials support requires
GNUTLS'\n",
The four bytes " \\\n " portion between the two lines are not
essential (the shell grammar doesn't care if there is a newline at
this point); in fact, if you put the | before \n instead of after, you
can use a newline without needing the \. But I don't see any point
changing this code yet again just to golf out a few more bytes. Let's
leave it as written in your v2.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
1:41 p.m.
On 4/19/23 20:21, Eric Blake wrote:
On Wed, Apr 19, 2023 at 05:31:27PM +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.
>
> At Eric's suggestion, also:
>
> - replace the non-portable control operator "|&" with the portable
> redirection operator "2>&1" and the portable control operator
"|";
>
> - replace the "if ...; then exit 1; else exit 0; fi" constructs with
"!
> ...".
Reviewed-by: Eric Blake <eblake(a)redhat.com>
Thanks!
> Notes:
> v2:
>
> - v1: https://listman.redhat.com/archives/libguestfs/2023-April/031298.html
>
> - incorporate Eric's recommendations (commit message, code)
>
> tests/requires.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/requires.c b/tests/requires.c
> index 199e30605473..2b7031532fe2 100644
> --- a/tests/requires.c
> +++ b/tests/requires.c
> @@ -57,7 +57,8 @@ 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",
> + "! %s --object tls-creds-x509,id=tls0 2>&1 \\\n"
> + " | grep -sq 'TLS credentials support requires
GNUTLS'\n",
The four bytes " \\\n " portion between the two lines are not
essential (the shell grammar doesn't care if there is a newline at
this point);
Indeed, but I wanted the shell script's line breaks to follow the C
source's line breaks. :)
in fact, if you put the | before \n instead of after, you
can use a newline without needing the \.
That's right, but personally I prefer
cmd1 \
| cmd2 \
| cmd3 \
| cmd4
over
cmd1 |
cmd2 |
cmd3 |
cmd4
because in the latter (especially if the commands are long / complex),
it's hard to see where a pipeline ends and a brand new pipeline starts.
Placing the pipe symbol at the front gives more visual cues.
(In C, things are different: whenever we need expressions spanning
multiple lines, we can easily keep the operators at the end of the line.
That's because we already have parens in place, e.g. because the
expression is the controlling expression of a statement, or because we
can simply add parens for the sake of indentation. But I've never seen
something like this, in shell:
{
cmd1 |
cmd2 |
cmd3 |
cmd4;
}
and anyway I think it would run the risk of the exact same mistake --
forgetting a pipe symbol somewhere would not cause a "compilation error"
but runtime misbehavior.)
But I don't see any point
changing this code yet again just to golf out a few more bytes. Let's
leave it as written in your v2.
Thanks! :) At least this time all of it was fully intended on my part.
Laszlo
3:41 a.m.
On 4/19/23 20:21, Eric Blake wrote:
On Wed, Apr 19, 2023 at 05:31:27PM +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.
>
> At Eric's suggestion, also:
>
> - replace the non-portable control operator "|&" with the portable
> redirection operator "2>&1" and the portable control operator
"|";
>
> - replace the "if ...; then exit 1; else exit 0; fi" constructs with
"!
> ...".
Reviewed-by: Eric Blake <eblake(a)redhat.com>
> Notes:
> v2:
>
> - v1: https://listman.redhat.com/archives/libguestfs/2023-April/031298.html
>
> - incorporate Eric's recommendations (commit message, code)
>
> tests/requires.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/requires.c b/tests/requires.c
> index 199e30605473..2b7031532fe2 100644
> --- a/tests/requires.c
> +++ b/tests/requires.c
> @@ -57,7 +57,8 @@ 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",
> + "! %s --object tls-creds-x509,id=tls0 2>&1 \\\n"
> + " | grep -sq 'TLS credentials support requires
GNUTLS'\n",
The four bytes " \\\n " portion between the two lines are not
essential (the shell grammar doesn't care if there is a newline at
this point); in fact, if you put the | before \n instead of after, you
can use a newline without needing the \. But I don't see any point
changing this code yet again just to golf out a few more bytes. Let's
leave it as written in your v2.
Commit 86820dbab497.
Thanks
Laszlo
581
days inactive
582
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Laszlo Ersek