| Message ID | 20190731091933.17363-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] net: tap: replace snprintf with g_strdup_printf calls | expand |
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
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 {
+-- 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
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 {