When rewriting a file using virFileRewrite() and error occurs while writing into a temporary file it's actually the callback that can report the most accurate error. Move error reporting into very few callback we have currently. Those callbacks are trivial so the benefit of this change is not obvious, but this will change shortly when slightly more complicated callback is introduced. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfile.c | 18 ++++++++++++------ src/util/virfile.h | 4 +++- src/util/virxml.c | 19 ++++++++++++++----- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index f99e7f95e1..5d6f14ba7e 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -497,7 +497,8 @@ int virFileUnlock(int fd G_GNUC_UNUSED, * temporary file on a side and renaming it to the desired name. * The temporary file is created using supplied @mode and * @uid:@gid (pass -1 for current uid/gid) and written by - * @rewrite callback. + * @rewrite callback. It's callback's responsibility to report + * errors. * * Returns: 0 on success, * -1 otherwise (with error reported) @@ -512,6 +513,7 @@ virFileRewrite(const char *path, g_autofree char *newfile = NULL; int fd = -1; int ret = -1; + int rc; newfile = g_strdup_printf("%s.new", path); @@ -524,9 +526,7 @@ virFileRewrite(const char *path, goto cleanup; } - if (rewrite(fd, opaque) < 0) { - virReportSystemError(errno, _("cannot write data to file '%s'"), - newfile); + if ((rc = rewrite(fd, newfile, opaque)) < 0) { goto cleanup; } @@ -558,12 +558,18 @@ virFileRewrite(const char *path, static int -virFileRewriteStrHelper(int fd, const void *opaque) +virFileRewriteStrHelper(int fd, + const char *path, + const void *opaque) { const char *data = opaque; - if (safewrite(fd, data, strlen(data)) < 0) + if (safewrite(fd, data, strlen(data)) < 0) { + virReportSystemError(errno, + _("cannot write data to file '%s'"), + path); return -1; + } return 0; } diff --git a/src/util/virfile.h b/src/util/virfile.h index 34184b32aa..b04386f6e6 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -123,7 +123,9 @@ int virFileLock(int fd, bool shared, off_t start, off_t len, bool waitForLock) int virFileUnlock(int fd, off_t start, off_t len) G_GNUC_NO_INLINE; -typedef int (*virFileRewriteFunc)(int fd, const void *opaque); +typedef int (*virFileRewriteFunc)(int fd, + const char *path, + const void *opaque); int virFileRewrite(const char *path, mode_t mode, uid_t uid, gid_t gid, diff --git a/src/util/virxml.c b/src/util/virxml.c index a55eb9629b..268aad1d20 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1172,17 +1172,26 @@ struct virXMLRewriteFileData { }; static int -virXMLRewriteFile(int fd, const void *opaque) +virXMLRewriteFile(int fd, + const char *path, + const void *opaque) { const struct virXMLRewriteFileData *data = opaque; - if (data->warnCommand) { - if (virXMLEmitWarning(fd, data->warnName, data->warnCommand) < 0) - return -1; + if (data->warnCommand && + virXMLEmitWarning(fd, data->warnName, data->warnCommand) < 0) { + virReportSystemError(errno, + _("cannot write data to file '%s'"), + path); + return -1; } - if (safewrite(fd, data->xml, strlen(data->xml)) < 0) + if (safewrite(fd, data->xml, strlen(data->xml)) < 0) { + virReportSystemError(errno, + _("cannot write data to file '%s'"), + path); return -1; + } return 0; } -- 2.34.1

After previous commits there is no need for qemuPrepareNVRAM() to open code virFileRewrite(). Deduplicate the code by calling the function. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_process.c | 112 ++++++++++++++++------------------------ 1 file changed, 44 insertions(+), 68 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8fccf6b760..036d95924c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4421,6 +4421,41 @@ qemuProcessUpdateCPU(virQEMUDriver *driver, } +struct qemuPrepareNVRAMHelperData { + int srcFD; + const char *srcPath; +}; + +static int +qemuPrepareNVRAMHelper(int dstFD, + const char *dstPath, + const void *opaque) +{ + const struct qemuPrepareNVRAMHelperData *data = opaque; + ssize_t r; + + do { + char buf[1024]; + + if ((r = saferead(data->srcFD, buf, sizeof(buf))) < 0) { + virReportSystemError(errno, + _("Unable to read from file '%s'"), + data->srcPath); + return -2; + } + + if (safewrite(dstFD, buf, r) < 0) { + virReportSystemError(errno, + _("Unable to write to file '%s'"), + dstPath); + return -1; + } + } while (r); + + return 0; +} + + static int qemuPrepareNVRAM(virQEMUDriver *driver, virDomainObj *vm, @@ -4429,12 +4464,9 @@ qemuPrepareNVRAM(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); int ret = -1; int srcFD = -1; - int dstFD = -1; virDomainLoaderDef *loader = vm->def->os.loader; - bool created = false; const char *master_nvram_path; - ssize_t r; - g_autofree char *tmp_dst_path = NULL; + struct qemuPrepareNVRAMHelperData data; if (!loader || !loader->nvram || (virFileExists(loader->nvram) && !reset_nvram)) @@ -4466,76 +4498,20 @@ qemuPrepareNVRAM(virQEMUDriver *driver, goto cleanup; } - tmp_dst_path = g_strdup_printf("%s.tmp", loader->nvram); - if ((dstFD = virFileOpenAs(tmp_dst_path, - O_WRONLY | O_CREAT | O_EXCL, - S_IRUSR | S_IWUSR, - cfg->user, cfg->group, - VIR_FILE_OPEN_FORCE_OWNER)) < 0) { - virReportSystemError(-dstFD, - _("Failed to create file '%s'"), - tmp_dst_path); - goto cleanup; - } - - created = true; - - do { - char buf[1024]; - - if ((r = saferead(srcFD, buf, sizeof(buf))) < 0) { - virReportSystemError(errno, - _("Unable to read from file '%s'"), - master_nvram_path); - goto cleanup; - } - - if (safewrite(dstFD, buf, r) < 0) { - virReportSystemError(errno, - _("Unable to write to file '%s'"), - tmp_dst_path); - goto cleanup; - } - } while (r); - - if (VIR_CLOSE(srcFD) < 0) { - virReportSystemError(errno, - _("Unable to close file '%s'"), - master_nvram_path); - goto cleanup; - } - - if (g_fsync(dstFD) < 0) { - virReportSystemError(errno, _("cannot sync file '%s'"), - tmp_dst_path); - goto cleanup; - } - - if (VIR_CLOSE(dstFD) < 0) { - virReportSystemError(errno, - _("Unable to close file '%s'"), - tmp_dst_path); - goto cleanup; - } - - if (rename(tmp_dst_path, loader->nvram) < 0) { - virReportSystemError(errno, - _("Unable to replace '%s'"), - loader->nvram); + data.srcFD = srcFD; + data.srcPath = master_nvram_path; + + if (virFileRewrite(loader->nvram, + S_IRUSR | S_IWUSR, + cfg->user, cfg->group, + qemuPrepareNVRAMHelper, + &data) < 0) { goto cleanup; } ret = 0; cleanup: - /* We successfully generated the nvram path, but failed to - * copy the file content. Roll back. */ - if (ret < 0) { - if (created) - unlink(tmp_dst_path); - } - VIR_FORCE_CLOSE(srcFD); - VIR_FORCE_CLOSE(dstFD); return ret; } -- 2.34.1

Nothing inside the qemuPrepareNVRAM function relies on @srcFD being closed early and nothing closes it early. It's okay then to close it automatically when leaving the function. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_process.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 036d95924c..0ba9343b2a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4463,7 +4463,7 @@ qemuPrepareNVRAM(virQEMUDriver *driver, { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); int ret = -1; - int srcFD = -1; + VIR_AUTOCLOSE srcFD = -1; virDomainLoaderDef *loader = vm->def->os.loader; const char *master_nvram_path; struct qemuPrepareNVRAMHelperData data; @@ -4511,7 +4511,6 @@ qemuPrepareNVRAM(virQEMUDriver *driver, ret = 0; cleanup: - VIR_FORCE_CLOSE(srcFD); return ret; } -- 2.34.1

After previous commits, the cleanup label shrank to plain 'return' statement. There's no point in having such label, so drop it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_process.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0ba9343b2a..0b9d96b7d5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4462,7 +4462,6 @@ qemuPrepareNVRAM(virQEMUDriver *driver, bool reset_nvram) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - int ret = -1; VIR_AUTOCLOSE srcFD = -1; virDomainLoaderDef *loader = vm->def->os.loader; const char *master_nvram_path; @@ -4487,7 +4486,7 @@ qemuPrepareNVRAM(virQEMUDriver *driver, virReportError(VIR_ERR_OPERATION_FAILED, _("unable to find any master var store for " "loader: %s"), loader->path); - goto cleanup; + return -1; } if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY, @@ -4495,7 +4494,7 @@ qemuPrepareNVRAM(virQEMUDriver *driver, virReportSystemError(-srcFD, _("Failed to open file '%s'"), master_nvram_path); - goto cleanup; + return -1; } data.srcFD = srcFD; @@ -4506,12 +4505,10 @@ qemuPrepareNVRAM(virQEMUDriver *driver, cfg->user, cfg->group, qemuPrepareNVRAMHelper, &data) < 0) { - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } -- 2.34.1