[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v4 04/45] tests: Replace fprintf(st
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v4 04/45] tests: Replace fprintf(stderr, "*\n" with error_report() |
Date: |
Thu, 9 Nov 2017 08:38:23 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
On 11/08/2017 04:56 PM, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
s/where/were/
>
> Some of the error_report()'s were manually kept as fprintf(stderr, ) to
> maintain standalone test cases.
>
> Signed-off-by: Alistair Francis <address@hidden>
> Cc: "Michael S. Tsirkin" <address@hidden>
> Cc: Igor Mammedov <address@hidden>
> Cc: "Dr. David Alan Gilbert" <address@hidden>
> Cc: Gerd Hoffmann <address@hidden>
> Cc: address@hidden
> ---
> V2:
> - Keep some of the fprintf() calls
> - Remove a file I accidently checked in
>
> +++ b/tests/ahci-test.c
> @@ -23,6 +23,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include <getopt.h>
>
> #include "libqtest.h"
> @@ -1859,7 +1860,7 @@ int main(int argc, char **argv)
> ahci_pedantic = 1;
> break;
> default:
> - fprintf(stderr, "Unrecognized ahci_test option.\n");
> + error_report("Unrecognized ahci_test option.");
Most of our error_report() calls do not include trailing dot.
> +++ b/tests/bios-tables-test.c
> @@ -11,6 +11,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include <glib/gstdio.h>
> #include "qemu-common.h"
> #include "hw/smbios/smbios.h"
> @@ -396,7 +397,7 @@ try_again:
> aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
> (gchar *)&signature, ext);
> if (getenv("V")) {
> - fprintf(stderr, "\nLooking for expected file '%s'\n", aml_file);
> + error_report("Looking for expected file '%s'", aml_file);
Here, it looks like this is a debug statement, gated by whether $V is
set in the environment; it's not really an error.
> }
> if (g_file_test(aml_file, G_FILE_TEST_EXISTS)) {
> exp_sdt.aml_file = aml_file;
> @@ -408,7 +409,7 @@ try_again:
> }
> g_assert(exp_sdt.aml_file);
> if (getenv("V")) {
> - fprintf(stderr, "\nUsing expected file '%s'\n", aml_file);
> + error_report("Using expected file '%s'", aml_file);
> }
Ditto.
> +++ b/tests/i440fx-test.c
> @@ -13,7 +13,7 @@
> */
>
> #include "qemu/osdep.h"
> -
> +#include "qemu/error-report.h"
> #include "libqtest.h"
> #include "libqos/pci.h"
> #include "libqos/pci-pc.h"
> @@ -295,18 +295,18 @@ static char *create_blob_file(void)
> ret = -1;
> fd = g_file_open_tmp("blob_XXXXXX", &pathname, &error);
> if (fd == -1) {
> - fprintf(stderr, "unable to create blob file: %s\n", error->message);
> + error_report("unable to create blob file: %s", error->message);
> g_error_free(error);
A candidate for
error_reportf_err(error, "unable to create blob file: ");
> +++ b/tests/libqos/ahci.c
> @@ -23,7 +23,7 @@
> */
>
> #include "qemu/osdep.h"
> -
> +#include "qemu/error-report.h"
> #include "libqtest.h"
> #include "libqos/ahci.h"
> #include "libqos/pci-pc.h"
> @@ -985,9 +985,9 @@ static void ahci_atapi_command_set_offset(AHCICommand
> *cmd, uint64_t lba)
> default:
> /* SCSI doesn't have uniform packet formats,
> * so you have to add support for it manually. Sorry! */
> - fprintf(stderr, "The Libqos AHCI driver does not support the "
> + error_report("The Libqos AHCI driver does not support the "
> "set_offset operation for ATAPI command 0x%02x, "
> - "please add support.\n",
> + "please add support.",
Trailing dot is unusual.
> cbd[0]);
> g_assert_not_reached();
> }
> @@ -1060,8 +1060,8 @@ static void ahci_atapi_set_size(AHCICommand *cmd,
> uint64_t xbytes)
> default:
> /* SCSI doesn't have uniform packet formats,
> * so you have to add support for it manually. Sorry! */
> - fprintf(stderr, "The Libqos AHCI driver does not support the
> set_size "
> - "operation for ATAPI command 0x%02x, please add support.\n",
> + error_report("The Libqos AHCI driver does not support the set_size "
> + "operation for ATAPI command 0x%02x, please add support.",
Again
> +++ b/tests/libqos/libqos.c
> @@ -199,7 +200,7 @@ void mkimg(const char *file, const char *fmt, unsigned
> size_mb)
> fmt, file, size_mb);
> ret = g_spawn_command_line_sync(cli, &out, &out2, &rc, &err);
> if (err) {
> - fprintf(stderr, "%s\n", err->message);
> + error_report("%s", err->message);
> g_error_free(err);
Candidate for error_report_err()
> +++ b/tests/libqos/malloc.c
> @@ -11,6 +11,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "libqos/malloc.h"
> #include "qemu-common.h"
> #include "qemu/host-utils.h"
> @@ -193,7 +194,7 @@ static uint64_t mlist_alloc(QGuestAllocator *s, uint64_t
> size)
>
> node = mlist_find_space(s->free, size);
> if (!node) {
> - fprintf(stderr, "Out of guest memory.\n");
> + error_report("Out of guest memory.");
trailing dot
> g_assert_not_reached();
> }
> return mlist_fulfill(s, node, size);
> @@ -209,8 +210,8 @@ static void mlist_free(QGuestAllocator *s, uint64_t addr)
>
> node = mlist_find_key(s->used, addr);
> if (!node) {
> - fprintf(stderr, "Error: no record found for an allocation at "
> - "0x%016" PRIx64 ".\n",
> + error_report("Error: no record found for an allocation at "
> + "0x%016" PRIx64 ".",
Again. I'll quit pointing these out.
> +++ b/tests/migration-test.c
> @@ -334,9 +334,9 @@ static void check_guests_ram(QTestState *who)
> */
> hit_edge = true;
> } else {
> - fprintf(stderr, "Memory content inconsistency at %x"
> + error_report("Memory content inconsistency at %x"
> " first_byte = %x last_byte = %x current =
> %x"
> - " hit_edge = %x\n",
> + " hit_edge = %x",
Indentation is now off.
> address, first_byte, last_byte, b, hit_edge);
> bad = true;
> }
> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
> index cf8ce8b16d..49e1ff4555 100644
> --- a/tests/migration/stress.c
> +++ b/tests/migration/stress.c
> @@ -47,7 +47,7 @@ static __attribute__((noreturn)) void exit_failure(void)
> if (getpid() == 1) {
> sync();
> reboot(RB_POWER_OFF);
> - fprintf(stderr, "%s (%05d): ERROR: cannot reboot: %s\n",
> + error_report("%s (%05d): cannot reboot: %s",
> argv0, gettid(), strerror(errno));
Indentation is now off.
> abort();
> } else {
> @@ -60,7 +60,7 @@ static __attribute__((noreturn)) void exit_success(void)
> if (getpid() == 1) {
> sync();
> reboot(RB_POWER_OFF);
> - fprintf(stderr, "%s (%05d): ERROR: cannot reboot: %s\n",
> + error_report("%s (%05d): cannot reboot: %s",
> argv0, gettid(), strerror(errno));
And again. I'll quit pointing it out.
> +++ b/tests/rcutorture.c
> @@ -61,6 +61,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "qemu/atomic.h"
> #include "qemu/rcu.h"
> #include "qemu/thread.h"
> @@ -86,7 +87,7 @@ static int n_threads;
> static void create_thread(void *(*func)(void *))
> {
> if (n_threads >= NR_THREADS) {
> - fprintf(stderr, "Thread limit of %d exceeded!\n", NR_THREADS);
> + error_report("Thread limit of %d exceeded!", NR_THREADS);
Trailing ! is shouting at the user; it's even less appropriate than
trailing dot.
> exit(-1);
> }
> qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads],
> @@ -417,7 +418,7 @@ static void gtest_stress_10_5(void)
>
> static void usage(int argc, char *argv[])
> {
> - fprintf(stderr, "Usage: %s [nreaders [ perf | stress ] ]\n", argv[0]);
> + error_report("Usage: %s [nreaders [ perf | stress ] ]", argv[0]);
> exit(-1);
Separate patch - but 'exit(-1)' is almost always wrong (it gives status
255 through wait()/waitpid(); meanwhile waitid() is required by POSIX to
get at the full 32-bit value except that Linux doesn't obey that
requirement). A process where wait() returns 255 makes xargs behave
differently. We really want exit(1).
> +++ b/tests/tcg/linux-test.c
> @@ -51,7 +51,7 @@ void error1(const char *filename, int line, const char
> *fmt, ...)
> va_start(ap, fmt);
> fprintf(stderr, "%s:%d: ", filename, line);
> vfprintf(stderr, fmt, ap);
> - fprintf(stderr, "\n");
> + error_report("");
Umm, a blank line is not a useful error. This hunk is bogus; we
probably want to stick with fprintf() for the entire message.
> +++ b/tests/tcg/test_path.c
> @@ -150,8 +150,8 @@ int main(int argc, char *argv[])
> ret = do_test();
> cleanup();
> if (ret) {
> - fprintf(stderr, "test_path: failed on line %i\n", ret);
> - return 1;
> + error_report("test_path: failed on line %i", ret);
> + return 1;
Yay for fixing TAB damage along the way.
> +++ b/tests/test-hmp.c
> @@ -15,6 +15,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "libqtest.h"
>
> static int verbose;
> @@ -79,7 +80,7 @@ static void test_commands(void)
>
> for (i = 0; hmp_cmds[i] != NULL; i++) {
> if (verbose) {
> - fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> + error_report("\t%s", hmp_cmds[i]);
Verbose messages aren't really errors. I don't think you want this hunk.
> }
> response = hmp("%s", hmp_cmds[i]);
> g_free(response);
> @@ -102,7 +103,7 @@ static void test_info_commands(void)
> *endp = '\0';
> /* Now run the info command */
> if (verbose) {
> - fprintf(stderr, "\t%s\n", info);
> + error_report("\t%s", info);
Likewise.
> +++ b/tests/test-rcu-list.c
> @@ -21,6 +21,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "qemu/atomic.h"
> #include "qemu/rcu.h"
> #include "qemu/thread.h"
> @@ -64,7 +65,7 @@ static int select_random_el(int max)
> static void create_thread(void *(*func)(void *))
> {
> if (n_threads >= NR_THREADS) {
> - fprintf(stderr, "Thread limit of %d exceeded!\n", NR_THREADS);
> + error_report("Thread limit of %d exceeded!", NR_THREADS);
> exit(-1);
Another !, another exit(-1)
> +++ b/tests/usb-hcd-ehci-test.c
> @@ -42,7 +42,7 @@ static void ehci_port_test(struct qhc *hc, int port,
> uint32_t expect)
> uint16_t mask = ~(PORTSC_CSC | PORTSC_PEDC | PORTSC_OCC);
>
> #if 0
> - fprintf(stderr, "%s: %d, have 0x%08x, want 0x%08x\n",
> + error_report("%s: %d, have 0x%08x, want 0x%08x",
> __func__, port, value & mask, expect & mask);
> #endif
I'd just nuke the #if 0 block entirely, as it is likely to bit-rot
(separate patch, though).
> +++ b/tests/vhost-user-bridge.c
> @@ -714,15 +714,15 @@ main(int argc, char *argv[])
> return 0;
>
> out:
> - fprintf(stderr, "Usage: %s ", argv[0]);
> - fprintf(stderr, "[-c] [-u ud_socket_path] [-l lhost:lport] [-r
> rhost:rport]\n");
> - fprintf(stderr, "\t-u path to unix doman socket. default: %s\n",
> + error_report("Usage: %s ", argv[0]);
> + error_report("[-c] [-u ud_socket_path] [-l lhost:lport] [-r
> rhost:rport]");
> + error_report("\t-u path to unix doman socket. default: %s",
> DEFAULT_UD_SOCKET);
> - fprintf(stderr, "\t-l local host and port. default: %s:%s\n",
> + fprintf(stderr, "\t-l local host and port. default: %s:%s",
> DEFAULT_LHOST, DEFAULT_LPORT);
> - fprintf(stderr, "\t-r remote host and port. default: %s:%s\n",
> + error_report("\t-r remote host and port. default: %s:%s",
> DEFAULT_RHOST, DEFAULT_RPORT);
> - fprintf(stderr, "\t-c client mode\n");
> + error_report("\t-c client mode");
Is usage text really an error?
>
> return 1;
> }
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature