* [Qemu-devel] [PATCH v5] net: tap: replace snprintf with g_strdup_printf calls
@ 2019-07-31 9:19 P J P
2019-07-31 9:35 ` no-reply
2019-07-31 11:36 ` Markus Armbruster
0 siblings, 2 replies; 4+ messages in thread
From: P J P @ 2019-07-31 9:19 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, Li Qiang, Daniel P . Berrangé,
QEMU Developers, Prasad J Pandit
From: Prasad J Pandit <pjp@fedoraproject.org>
When invoking qemu-bridge-helper in 'net_bridge_run_helper',
instead of using fixed sized buffers, use dynamically allocated
ones initialised and returned by g_strdup_printf().
If bridge name 'br_buf' is undefined, pass empty string ("") to
g_strdup_printf() in its place, to avoid printing "(null)" string.
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
net/tap.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
Update v5: add commit message about conditional 'br_buf' argument
-> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg06397.html
diff --git a/net/tap.c b/net/tap.c
index e8aadd8d4b..fc38029f41 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -498,9 +498,9 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
}
if (pid == 0) {
int open_max = sysconf(_SC_OPEN_MAX), i;
- char fd_buf[6+10];
- char br_buf[6+IFNAMSIZ] = {0};
- char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
+ char *fd_buf = NULL;
+ char *br_buf = NULL;
+ char *helper_cmd = NULL;
for (i = 3; i < open_max; i++) {
if (i != sv[1]) {
@@ -508,17 +508,17 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
}
}
- snprintf(fd_buf, sizeof(fd_buf), "%s%d", "--fd=", sv[1]);
+ fd_buf = g_strdup_printf("%s%d", "--fd=", sv[1]);
if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
/* assume helper is a command */
if (strstr(helper, "--br=") == NULL) {
- snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
+ br_buf = g_strdup_printf("%s%s", "--br=", bridge);
}
- snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
- helper, "--use-vnet", fd_buf, br_buf);
+ helper_cmd = g_strdup_printf("%s %s %s %s", helper,
+ "--use-vnet", fd_buf, br_buf ? br_buf : "");
parg = args;
*parg++ = (char *)"sh";
@@ -527,10 +527,11 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
*parg++ = NULL;
execv("/bin/sh", args);
+ g_free(helper_cmd);
} else {
/* assume helper is just the executable path name */
- snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
+ br_buf = g_strdup_printf("%s%s", "--br=", bridge);
parg = args;
*parg++ = (char *)helper;
@@ -541,6 +542,8 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
execv(helper, args);
}
+ g_free(fd_buf);
+ g_free(br_buf);
_exit(1);
} else {
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v5] net: tap: replace snprintf with g_strdup_printf calls
2019-07-31 9:19 [Qemu-devel] [PATCH v5] net: tap: replace snprintf with g_strdup_printf calls P J P
@ 2019-07-31 9:35 ` no-reply
2019-07-31 11:36 ` Markus Armbruster
1 sibling, 0 replies; 4+ messages in thread
From: no-reply @ 2019-07-31 9:35 UTC (permalink / raw)
To: ppandit; +Cc: berrange, pjp, stefanha, jasowang, liq3ea, qemu-devel
Patchew URL: https://patchew.org/QEMU/20190731091933.17363-1-ppandit@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH v5] net: tap: replace snprintf with g_strdup_printf calls
Message-id: 20190731091933.17363-1-ppandit@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20190731091933.17363-1-ppandit@redhat.com -> patchew/20190731091933.17363-1-ppandit@redhat.com
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp'
Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out 'ba1ab360eebe6338bb8d7d83a9220ccf7e213af3'
Cloning into 'roms/edk2'...
Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54'
Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl'
Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687'
Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl'
Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography'
Cloning into 'boringssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
Cloning into 'krb5'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168'
Cloning into 'pyca-cryptography'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/opensbi'...
Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'slirp'...
Submodule path 'slirp': checked out 'f0da6726207b740f6101028b2992f918477a4b08'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
=== OUTPUT BEGIN ===
checkpatch.pl: no revisions returned for revlist '1'
=== OUTPUT END ===
Test command exited with code: 255
The full log is available at
http://patchew.org/logs/20190731091933.17363-1-ppandit@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v5] net: tap: replace snprintf with g_strdup_printf calls
2019-07-31 9:19 [Qemu-devel] [PATCH v5] net: tap: replace snprintf with g_strdup_printf calls P J P
2019-07-31 9:35 ` no-reply
@ 2019-07-31 11:36 ` Markus Armbruster
2019-08-01 7:57 ` P J P
1 sibling, 1 reply; 4+ messages in thread
From: Markus Armbruster @ 2019-07-31 11:36 UTC (permalink / raw)
To: P J P
Cc: Daniel P . Berrangé,
Prasad J Pandit, Stefan Hajnoczi, Jason Wang, Li Qiang,
QEMU Developers
P J P <ppandit@redhat.com> writes:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> When invoking qemu-bridge-helper in 'net_bridge_run_helper',
> instead of using fixed sized buffers, use dynamically allocated
> ones initialised and returned by g_strdup_printf().
Does this fix a bug?
> If bridge name 'br_buf' is undefined, pass empty string ("") to
> g_strdup_printf() in its place, to avoid printing "(null)" string.
>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> net/tap.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> Update v5: add commit message about conditional 'br_buf' argument
> -> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg06397.html
>
> diff --git a/net/tap.c b/net/tap.c
> index e8aadd8d4b..fc38029f41 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -498,9 +498,9 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
> }
> if (pid == 0) {
> int open_max = sysconf(_SC_OPEN_MAX), i;
> - char fd_buf[6+10];
> - char br_buf[6+IFNAMSIZ] = {0};
> - char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
> + char *fd_buf = NULL;
Dead initializer.
> + char *br_buf = NULL;
> + char *helper_cmd = NULL;
Another one.
>
> for (i = 3; i < open_max; i++) {
> if (i != sv[1]) {
> @@ -508,17 +508,17 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
> }
> }
>
> - snprintf(fd_buf, sizeof(fd_buf), "%s%d", "--fd=", sv[1]);
> + fd_buf = g_strdup_printf("%s%d", "--fd=", sv[1]);
Good opportunity to change this to
fd_buf = g_strdup_printf("--fd=%d", sv[1]);
More of the same below.
>
> if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
> /* assume helper is a command */
>
> if (strstr(helper, "--br=") == NULL) {
> - snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> + br_buf = g_strdup_printf("%s%s", "--br=", bridge);
> }
>
> - snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
> - helper, "--use-vnet", fd_buf, br_buf);
> + helper_cmd = g_strdup_printf("%s %s %s %s", helper,
> + "--use-vnet", fd_buf, br_buf ? br_buf : "");
>
> parg = args;
> *parg++ = (char *)"sh";
> @@ -527,10 +527,11 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
> *parg++ = NULL;
>
> execv("/bin/sh", args);
> + g_free(helper_cmd);
> } else {
> /* assume helper is just the executable path name */
>
> - snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> + br_buf = g_strdup_printf("%s%s", "--br=", bridge);
>
> parg = args;
> *parg++ = (char *)helper;
> @@ -541,6 +542,8 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
>
> execv(helper, args);
> }
> + g_free(fd_buf);
> + g_free(br_buf);
> _exit(1);
>
> } else {
The commit does what it claims to do, and no more, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>
However, the code is still rather ugly, and I'd be tempted to use the
opportunity to clean up some more. Untested sketch:
diff --git a/net/tap.c b/net/tap.c
index 06af8fb8ad..57bb4c552d 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -402,8 +402,7 @@ static void launch_script(const char *setup_script, const char *ifname,
int fd, Error **errp)
{
int pid, status;
- char *args[3];
- char **parg;
+ const char *argv[3];
/* try to launch network script */
pid = fork();
@@ -413,18 +412,18 @@ static void launch_script(const char *setup_script, const char *ifname,
return;
}
if (pid == 0) {
- int open_max = sysconf(_SC_OPEN_MAX), i;
+ int open_max = sysconf(_SC_OPEN_MAX);
+ int i;
for (i = 3; i < open_max; i++) {
if (i != fd) {
close(i);
}
}
- parg = args;
- *parg++ = (char *)setup_script;
- *parg++ = (char *)ifname;
- *parg = NULL;
- execv(setup_script, args);
+ argv[0] = setup_script;
+ argv[1] = ifname;
+ argv[2] = NULL;
+ execv(setup_script, (char *const *)argv);
_exit(1);
} else {
while (waitpid(pid, &status, 0) != pid) {
@@ -478,8 +477,7 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
{
sigset_t oldmask, mask;
int pid, status;
- char *args[5];
- char **parg;
+ const char *argv[5];
int sv[2];
sigemptyset(&mask);
@@ -498,10 +496,9 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
return -1;
}
if (pid == 0) {
- int open_max = sysconf(_SC_OPEN_MAX), i;
- char fd_buf[6+10];
- char br_buf[6+IFNAMSIZ] = {0};
- char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
+ int open_max = sysconf(_SC_OPEN_MAX);
+ int i;
+ char *fd_opt, *br_opt;
for (i = 3; i < open_max; i++) {
if (i != sv[1]) {
@@ -509,39 +506,30 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
}
}
- snprintf(fd_buf, sizeof(fd_buf), "%s%d", "--fd=", sv[1]);
+ fd_opt = g_strdup_printf("--fd=%d", sv[1]);
+ br_opt = g_strdup_printf("--br=%s", bridge);
if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
/* assume helper is a command */
-
- if (strstr(helper, "--br=") == NULL) {
- snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
- }
-
- snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
- helper, "--use-vnet", fd_buf, br_buf);
-
- parg = args;
- *parg++ = (char *)"sh";
- *parg++ = (char *)"-c";
- *parg++ = helper_cmd;
- *parg++ = NULL;
-
- execv("/bin/sh", args);
+ /* FIXME strstr() is lazy and somewhat fragile */
+ bool need_br = !strstr(helper, "--br=");
+
+ argv[0] = "/bin/sh";
+ argv[1] = "-c";
+ argv[2] = g_strdup_printf("%s --use-vnet %s %s",
+ helper, fd_opt,
+ need_br ? br_opt : "");
+ argv[3] = NULL;
} else {
/* assume helper is just the executable path name */
-
- snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
-
- parg = args;
- *parg++ = (char *)helper;
- *parg++ = (char *)"--use-vnet";
- *parg++ = fd_buf;
- *parg++ = br_buf;
- *parg++ = NULL;
-
- execv(helper, args);
+ argv[0] = helper;
+ argv[1] = "--use-vnet";
+ argv[2] = fd_opt;
+ argv[3] = br_opt;
+ argv[4] = NULL;
}
+
+ execv(argv[0], (char *const *)argv);
_exit(1);
} else {
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v5] net: tap: replace snprintf with g_strdup_printf calls
2019-07-31 11:36 ` Markus Armbruster
@ 2019-08-01 7:57 ` P J P
0 siblings, 0 replies; 4+ messages in thread
From: P J P @ 2019-08-01 7:57 UTC (permalink / raw)
To: Markus Armbruster
Cc: Stefan Hajnoczi, Jason Wang, Li Qiang, Daniel P . Berrangé,
QEMU Developers
+-- On Wed, 31 Jul 2019, Markus Armbruster wrote --+
| However, the code is still rather ugly, and I'd be tempted to use the
| opportunity to clean up some more. Untested sketch:
Patch v3 did a similar change
-> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00578.html
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-01 7:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31 9:19 [Qemu-devel] [PATCH v5] net: tap: replace snprintf with g_strdup_printf calls P J P
2019-07-31 9:35 ` no-reply
2019-07-31 11:36 ` Markus Armbruster
2019-08-01 7:57 ` P J P
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.