The previous code duplicated a lot of common code for reading and writing the cache file per data field. This change simply factors out that common code. This makes it simpler to add new tests in future. This is just refactoring, it should have no effect. --- lib/qemu.c | 383 +++++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 246 insertions(+), 137 deletions(-) diff --git a/lib/qemu.c b/lib/qemu.c index 48e283d48..f7536ec76 100644 --- a/lib/qemu.c +++ b/lib/qemu.c @@ -37,6 +37,7 @@ #include <libxml/uri.h> +#include "full-write.h" #include "ignore-value.h" #include "guestfs.h" @@ -44,6 +45,10 @@ #include "guestfs_protocol.h" struct qemu_data { + int generation; /* MEMO_GENERATION read from qemu.stat */ + uint64_t prev_size; /* Size of qemu binary when cached. */ + uint64_t prev_mtime; /* mtime of qemu binary when cached. */ + char *qemu_help; /* Output of qemu -help. */ char *qemu_devices; /* Output of qemu -device ? */ @@ -51,9 +56,45 @@ struct qemu_data { struct version qemu_version; /* Parsed qemu version number. */ }; -static int test_qemu (guestfs_h *g, struct qemu_data *data); +static int test_qemu_help (guestfs_h *g, struct qemu_data *data); +static int read_cache_qemu_help (guestfs_h *g, struct qemu_data *data, const char *filename); +static int write_cache_qemu_help (guestfs_h *g, const struct qemu_data *data, const char *filename); +static int test_qemu_devices (guestfs_h *g, struct qemu_data *data); +static int read_cache_qemu_devices (guestfs_h *g, struct qemu_data *data, const char *filename); +static int write_cache_qemu_devices (guestfs_h *g, const struct qemu_data *data, const char *filename); +static int read_cache_qemu_stat (guestfs_h *g, struct qemu_data *data, const char *filename); +static int write_cache_qemu_stat (guestfs_h *g, const struct qemu_data *data, const char *filename); static void parse_qemu_version (guestfs_h *g, const char *, struct version *qemu_version); static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len); +static int generic_read_cache (guestfs_h *g, const char *filename, char **strp); +static int generic_write_cache (guestfs_h *g, const char *filename, const char *str); + +/* This structure abstracts the data we are reading from qemu and how + * we get it. + */ +static const struct qemu_fields { + const char *name; + + /* Function to perform the test on g->hv. This must set the correct + * data->[field] to non-NULL, or else return an error. + */ + int (*test) (guestfs_h *g, struct qemu_data *data); + + /* Functions to read and write the cache file. + * read_cache returns -1 = error, 0 = no cache, 1 = cache data read. + * write_cache returns -1 = error, 0 = success. + */ + int (*read_cache) (guestfs_h *g, struct qemu_data *data, + const char *filename); + int (*write_cache) (guestfs_h *g, const struct qemu_data *data, + const char *filename); +} qemu_fields[] = { + { "help", + test_qemu_help, read_cache_qemu_help, write_cache_qemu_help }, + { "devices", + test_qemu_devices, read_cache_qemu_devices, write_cache_qemu_devices }, +}; +#define NR_FIELDS (sizeof qemu_fields / sizeof qemu_fields[0]) /* This is saved in the qemu.stat file, so if we decide to change the * test_qemu memoization format/data in future, we should increment @@ -63,9 +104,9 @@ static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len); #define MEMO_GENERATION 1 /** - * Test qemu binary (or wrapper) runs, and do C<qemu -help> so we know - * the version of qemu what options this qemu supports, and - * C<qemu -device ?> so we know what devices are available. + * Test that the qemu binary (or wrapper) runs, and do C<qemu -help> + * and other commands so we can find out the version of qemu and what + * options this qemu supports. * * This caches the results in the cachedir so that as long as the qemu * binary does not change, calling this is effectively free. @@ -73,12 +114,12 @@ static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len); struct qemu_data * guestfs_int_test_qemu (guestfs_h *g) { - struct qemu_data *data; struct stat statbuf; - CLEANUP_FREE char *cachedir = NULL, *qemu_stat_filename = NULL, - *qemu_help_filename = NULL, *qemu_devices_filename = NULL; - int generation; - uint64_t prev_size, prev_mtime; + struct qemu_data *data; + CLEANUP_FREE char *cachedir = NULL; + CLEANUP_FREE char *stat_filename = NULL; + int r; + size_t i; if (stat (g->hv, &statbuf) == -1) { perrorf (g, "stat: %s", g->hv); @@ -89,165 +130,198 @@ guestfs_int_test_qemu (guestfs_h *g) if (cachedir == NULL) return NULL; - qemu_stat_filename = safe_asprintf (g, "%s/qemu.stat", cachedir); - qemu_help_filename = safe_asprintf (g, "%s/qemu.help", cachedir); - qemu_devices_filename = safe_asprintf (g, "%s/qemu.devices", cachedir); - /* Did we previously test the same version of qemu? */ debug (g, "checking for previously cached test results of %s, in %s", g->hv, cachedir); - { - CLEANUP_FCLOSE FILE *fp = NULL; - fp = fopen (qemu_stat_filename, "r"); - if (fp == NULL) - goto do_test; - if (fscanf (fp, "%d %" SCNu64 " %" SCNu64, - &generation, &prev_size, &prev_mtime) != 3) { - goto do_test; - } - } - - if (generation == MEMO_GENERATION && - (uint64_t) statbuf.st_size == prev_size && - (uint64_t) statbuf.st_mtime == prev_mtime) { - /* Same binary as before, so read the previously cached qemu -help - * and qemu -devices ? output. - */ - if (access (qemu_help_filename, R_OK) == -1 || - access (qemu_devices_filename, R_OK) == -1) - goto do_test; - - debug (g, "loading previously cached test results"); - - data = safe_calloc (g, 1, sizeof *data); - - if (guestfs_int_read_whole_file (g, qemu_help_filename, - &data->qemu_help, NULL) == -1) { - guestfs_int_free_qemu_data (data); - return NULL; - } - - parse_qemu_version (g, data->qemu_help, &data->qemu_version); - - if (guestfs_int_read_whole_file (g, qemu_devices_filename, - &data->qemu_devices, NULL) == -1) { - guestfs_int_free_qemu_data (data); - return NULL; - } - - return data; - } - - do_test: data = safe_calloc (g, 1, sizeof *data); - if (test_qemu (g, data) == -1) { - guestfs_int_free_qemu_data (data); - return NULL; + stat_filename = safe_asprintf (g, "%s/qemu.stat", cachedir); + r = read_cache_qemu_stat (g, data, stat_filename); + if (r == -1) + goto error; + if (r == 0) + goto do_test; + + if (data->generation != MEMO_GENERATION || + data->prev_size != (uint64_t) statbuf.st_size || + data->prev_mtime != (uint64_t) statbuf.st_mtime) + goto do_test; + + debug (g, "loading previously cached test results"); + + for (i = 0; i < NR_FIELDS; ++i) { + CLEANUP_FREE char *filename = + safe_asprintf (g, "%s/qemu.%s", cachedir, qemu_fields[i].name); + r = qemu_fields[i].read_cache (g, data, filename); + if (r == -1) + goto error; + if (r == 0) /* cache gone, maybe deleted by the tmp cleaner */ + goto do_test; } - parse_qemu_version (g, data->qemu_help, &data->qemu_version); + goto out; + + do_test: + for (i = 0; i < NR_FIELDS; ++i) { + if (qemu_fields[i].test (g, data) == -1) + goto error; + } /* Now memoize the qemu output in the cache directory. */ debug (g, "saving test results"); - { - CLEANUP_FCLOSE FILE *fp = NULL; - fp = fopen (qemu_help_filename, "w"); - if (fp == NULL) { - help_error: - perrorf (g, "%s", qemu_help_filename); - guestfs_int_free_qemu_data (data); - return NULL; - } - if (fprintf (fp, "%s", data->qemu_help) == -1) - goto help_error; + for (i = 0; i < NR_FIELDS; ++i) { + CLEANUP_FREE char *filename = + safe_asprintf (g, "%s/qemu.%s", cachedir, qemu_fields[i].name); + if (qemu_fields[i].write_cache (g, data, filename) == -1) + goto error; } - { - CLEANUP_FCLOSE FILE *fp = NULL; - fp = fopen (qemu_devices_filename, "w"); - if (fp == NULL) { - devices_error: - perrorf (g, "%s", qemu_devices_filename); - guestfs_int_free_qemu_data (data); - return NULL; - } - if (fprintf (fp, "%s", data->qemu_devices) == -1) - goto devices_error; - } + /* Write the qemu.stat file last so that its presence indicates that + * the qemu.help and qemu.devices files ought to exist. + */ + data->generation = MEMO_GENERATION; + data->prev_size = statbuf.st_size; + data->prev_mtime = statbuf.st_mtime; + if (write_cache_qemu_stat (g, data, stat_filename) == -1) + goto error; - { - /* Write the qemu.stat file last so that its presence indicates that - * the qemu.help and qemu.devices files ought to exist. - */ - CLEANUP_FCLOSE FILE *fp = NULL; - fp = fopen (qemu_stat_filename, "w"); - if (fp == NULL) { - stat_error: - perrorf (g, "%s", qemu_stat_filename); - guestfs_int_free_qemu_data (data); - return NULL; - } - /* The path to qemu is stored for information only, it is not - * used when we parse the file. - */ - if (fprintf (fp, "%d %" PRIu64 " %" PRIu64 " %s\n", - MEMO_GENERATION, - (uint64_t) statbuf.st_size, - (uint64_t) statbuf.st_mtime, - g->hv) == -1) - goto stat_error; - } + out: + /* Derived fields. */ + parse_qemu_version (g, data->qemu_help, &data->qemu_version); return data; + + error: + guestfs_int_free_qemu_data (data); + return NULL; } static int -test_qemu (guestfs_h *g, struct qemu_data *data) +test_qemu_help (guestfs_h *g, struct qemu_data *data) { - CLEANUP_CMD_CLOSE struct command *cmd1 = guestfs_int_new_command (g); - CLEANUP_CMD_CLOSE struct command *cmd2 = guestfs_int_new_command (g); + CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g); int r; - guestfs_int_cmd_add_arg (cmd1, g->hv); - guestfs_int_cmd_add_arg (cmd1, "-display"); - guestfs_int_cmd_add_arg (cmd1, "none"); - guestfs_int_cmd_add_arg (cmd1, "-help"); - guestfs_int_cmd_set_stdout_callback (cmd1, read_all, &data->qemu_help, - CMD_STDOUT_FLAG_WHOLE_BUFFER); - r = guestfs_int_cmd_run (cmd1); - if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0) - goto error; + guestfs_int_cmd_add_arg (cmd, g->hv); + guestfs_int_cmd_add_arg (cmd, "-display"); + guestfs_int_cmd_add_arg (cmd, "none"); + guestfs_int_cmd_add_arg (cmd, "-help"); + guestfs_int_cmd_set_stdout_callback (cmd, read_all, &data->qemu_help, + CMD_STDOUT_FLAG_WHOLE_BUFFER); + r = guestfs_int_cmd_run (cmd); + if (r == -1) + return -1; + if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { + guestfs_int_external_command_failed (g, r, g->hv, NULL); + return -1; + } + return 0; +} + +static int +read_cache_qemu_help (guestfs_h *g, struct qemu_data *data, + const char *filename) +{ + return generic_read_cache (g, filename, &data->qemu_help); +} + +static int +write_cache_qemu_help (guestfs_h *g, const struct qemu_data *data, + const char *filename) +{ + return generic_write_cache (g, filename, data->qemu_help); +} + +static int +test_qemu_devices (guestfs_h *g, struct qemu_data *data) +{ + CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g); + int r; - guestfs_int_cmd_add_arg (cmd2, g->hv); - guestfs_int_cmd_add_arg (cmd2, "-display"); - guestfs_int_cmd_add_arg (cmd2, "none"); - guestfs_int_cmd_add_arg (cmd2, "-machine"); - guestfs_int_cmd_add_arg (cmd2, + guestfs_int_cmd_add_arg (cmd, g->hv); + guestfs_int_cmd_add_arg (cmd, "-display"); + guestfs_int_cmd_add_arg (cmd, "none"); + guestfs_int_cmd_add_arg (cmd, "-machine"); + guestfs_int_cmd_add_arg (cmd, #ifdef MACHINE_TYPE MACHINE_TYPE "," #endif "accel=kvm:tcg"); - guestfs_int_cmd_add_arg (cmd2, "-device"); - guestfs_int_cmd_add_arg (cmd2, "?"); - guestfs_int_cmd_clear_capture_errors (cmd2); - guestfs_int_cmd_set_stderr_to_stdout (cmd2); - guestfs_int_cmd_set_stdout_callback (cmd2, read_all, &data->qemu_devices, - CMD_STDOUT_FLAG_WHOLE_BUFFER); - r = guestfs_int_cmd_run (cmd2); - if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0) - goto error; - - return 0; - - error: + guestfs_int_cmd_add_arg (cmd, "-device"); + guestfs_int_cmd_add_arg (cmd, "?"); + guestfs_int_cmd_clear_capture_errors (cmd); + guestfs_int_cmd_set_stderr_to_stdout (cmd); + guestfs_int_cmd_set_stdout_callback (cmd, read_all, &data->qemu_devices, + CMD_STDOUT_FLAG_WHOLE_BUFFER); + r = guestfs_int_cmd_run (cmd); if (r == -1) return -1; + if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { + guestfs_int_external_command_failed (g, r, g->hv, NULL); + return -1; + } + return 0; +} - guestfs_int_external_command_failed (g, r, g->hv, NULL); - return -1; +static int +read_cache_qemu_devices (guestfs_h *g, struct qemu_data *data, + const char *filename) +{ + return generic_read_cache (g, filename, &data->qemu_devices); +} + +static int +write_cache_qemu_devices (guestfs_h *g, const struct qemu_data *data, + const char *filename) +{ + return generic_write_cache (g, filename, data->qemu_devices); +} + +static int +read_cache_qemu_stat (guestfs_h *g, struct qemu_data *data, + const char *filename) +{ + CLEANUP_FCLOSE FILE *fp = fopen (filename, "r"); + if (fp == NULL) { + if (errno == ENOENT) + return 0; /* no cache, run the test instead */ + perrorf (g, "%s", filename); + return -1; + } + + if (fscanf (fp, "%d %" SCNu64 " %" SCNu64, + &data->generation, + &data->prev_size, + &data->prev_mtime) != 3) + return 0; + + return 1; +} + +static int +write_cache_qemu_stat (guestfs_h *g, const struct qemu_data *data, + const char *filename) +{ + CLEANUP_FCLOSE FILE *fp = fopen (filename, "w"); + if (fp == NULL) { + perrorf (g, "%s", filename); + return -1; + } + /* The path to qemu is stored for information only, it is not + * used when we parse the file. + */ + if (fprintf (fp, "%d %" PRIu64 " %" PRIu64 " %s\n", + data->generation, + data->prev_size, + data->prev_mtime, + g->hv) == -1) { + perrorf (g, "%s: write", filename); + return -1; + } + + return 0; } /** @@ -267,6 +341,41 @@ parse_qemu_version (guestfs_h *g, const char *qemu_help, } } +/** + * Generic functions for reading and writing the cache files, used + * where we are just reading and writing plain text strings. + */ +static int +generic_read_cache (guestfs_h *g, const char *filename, char **strp) +{ + if (access (filename, R_OK) == -1 && errno == ENOENT) + return 0; /* no cache, run the test instead */ + if (guestfs_int_read_whole_file (g, filename, strp, NULL) == -1) + return -1; + return 1; +} + +static int +generic_write_cache (guestfs_h *g, const char *filename, const char *str) +{ + CLEANUP_CLOSE int fd = -1; + size_t len; + + fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY|O_CLOEXEC, 0666); + if (fd == -1) { + perrorf (g, "%s", filename); + return -1; + } + + len = strlen (str); + if (full_write (fd, str, len) != len) { + perrorf (g, "%s: write", filename); + return -1; + } + + return 0; +} + static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len) { -- 2.13.2

Rename the cache files like ‘qemu.stat’ etc so they include the qemu binary "key" (ie. size and mtime) in the name. This allows a single user to use multiple qemu binaries in parallel without conflicts. --- lib/qemu.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/lib/qemu.c b/lib/qemu.c index 097d56929..1549bb33a 100644 --- a/lib/qemu.c +++ b/lib/qemu.c @@ -60,6 +60,7 @@ struct qemu_data { yajl_val qmp_schema_tree; /* qmp_schema parsed into a JSON tree */ }; +static char *cache_filename (guestfs_h *g, const char *cachedir, const struct stat *, const char *suffix); static int test_qemu_help (guestfs_h *g, struct qemu_data *data); static int read_cache_qemu_help (guestfs_h *g, struct qemu_data *data, const char *filename); static int write_cache_qemu_help (guestfs_h *g, const struct qemu_data *data, const char *filename); @@ -107,12 +108,12 @@ static const struct qemu_fields { }; #define NR_FIELDS (sizeof qemu_fields / sizeof qemu_fields[0]) -/* This is saved in the qemu.stat file, so if we decide to change the +/* This is saved in the qemu-*.stat file, so if we decide to change the * test_qemu memoization format/data in future, we should increment * this to discard any memoized data cached by previous versions of * libguestfs. */ -#define MEMO_GENERATION 2 +#define MEMO_GENERATION 3 /** * Test that the qemu binary (or wrapper) runs, and do C<qemu -help> @@ -147,7 +148,7 @@ guestfs_int_test_qemu (guestfs_h *g) data = safe_calloc (g, 1, sizeof *data); - stat_filename = safe_asprintf (g, "%s/qemu.stat", cachedir); + stat_filename = cache_filename (g, cachedir, &statbuf, "stat"); r = read_cache_qemu_stat (g, data, stat_filename); if (r == -1) goto error; @@ -163,7 +164,7 @@ guestfs_int_test_qemu (guestfs_h *g) for (i = 0; i < NR_FIELDS; ++i) { CLEANUP_FREE char *filename = - safe_asprintf (g, "%s/qemu.%s", cachedir, qemu_fields[i].name); + cache_filename (g, cachedir, &statbuf, qemu_fields[i].name); r = qemu_fields[i].read_cache (g, data, filename); if (r == -1) goto error; @@ -192,7 +193,7 @@ guestfs_int_test_qemu (guestfs_h *g) for (i = 0; i < NR_FIELDS; ++i) { CLEANUP_FREE char *filename = - safe_asprintf (g, "%s/qemu.%s", cachedir, qemu_fields[i].name); + cache_filename (g, cachedir, &statbuf, qemu_fields[i].name); if (qemu_fields[i].write_cache (g, data, filename) == -1) goto error; } @@ -218,6 +219,24 @@ guestfs_int_test_qemu (guestfs_h *g) return NULL; } +/** + * Generate the filenames, for the stat file and the other cache + * files. + * + * By including the size and mtime in the filename we also ensure that + * the same user can use multiple versions of qemu without conflicts. + */ +static char * +cache_filename (guestfs_h *g, const char *cachedir, + const struct stat *statbuf, const char *suffix) +{ + return safe_asprintf (g, "%s/qemu-%" PRIu64 "-%" PRIu64 ".%s", + cachedir, + (uint64_t) statbuf->st_size, + (uint64_t) statbuf->st_mtime, + suffix); +} + static int test_qemu_help (guestfs_h *g, struct qemu_data *data) { -- 2.13.2

--- lib/launch-direct.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/launch-direct.c b/lib/launch-direct.c index 9f85013f9..00cb25077 100644 --- a/lib/launch-direct.c +++ b/lib/launch-direct.c @@ -66,6 +66,7 @@ struct backend_direct_data { pid_t recoverypid; /* Recovery process PID. */ struct version qemu_version; /* qemu version (0 if unable to parse). */ + int qemu_mandatory_locking; /* qemu >= 2.10 does mandatory locking */ struct qemu_data *qemu_data; /* qemu -help output etc. */ char guestfsd_sock[UNIX_PATH_MAX]; /* Path to daemon socket. */ @@ -255,11 +256,13 @@ add_drive_standard_params (guestfs_h *g, struct backend_direct_data *data, } else { /* Writable qcow2 overlay on top of read-only drive. */ - append_list_format ("file=%s", drv->overlay); + append_list_format ("file.file.filename=%s", drv->overlay); + append_list ("file.driver=qcow2"); append_list ("cache=unsafe"); - append_list ("format=qcow2"); if (drv->disk_label) append_list_format ("serial=%s", drv->disk_label); + if (data->qemu_mandatory_locking) + append_list ("file.backing.file.locking=off"); } append_list_format ("id=hd%zu", i); @@ -408,6 +411,10 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) data->qemu_version = guestfs_int_qemu_version (g, data->qemu_data); debug (g, "qemu version: %d.%d", data->qemu_version.v_major, data->qemu_version.v_minor); + data->qemu_mandatory_locking = + guestfs_int_qemu_mandatory_locking (g, data->qemu_data); + debug (g, "qemu mandatory locking: %s", + data->qemu_mandatory_locking ? "yes" : "no"); } /* Using virtio-serial, we need to create a local Unix domain socket -- 2.13.2