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@redhat.com>
---
src/qemu/qemu_process.c | 118 +++++++++++++---------------------------
1 file changed, 39 insertions(+), 79 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8fccf6b760..500a91f153 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4421,6 +4421,40 @@ qemuProcessUpdateCPU(virQEMUDriver *driver,
}
+static int
+qemuPrepareNVRAMHelper(int dstFD,
+ const void *opaque)
+{
+ const char *master_nvram_path = opaque;
+ VIR_AUTOCLOSE srcFD = -1;
+ ssize_t r;
+
+ if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
+ 0, -1, -1, 0)) < 0) {
+ virReportSystemError(-srcFD,
+ _("Failed to open file '%s'"),
+ master_nvram_path);
+ return -2;
+ }
+
+ do {
+ char buf[1024];
+
+ if ((r = saferead(srcFD, buf, sizeof(buf))) < 0) {
+ virReportSystemError(errno,
+ _("Unable to read from file '%s'"),
+ master_nvram_path);
+ return -2;
+ }
+
+ if (safewrite(dstFD, buf, r) < 0)
+ return -1;
+ } while (r);
+
+ return 0;
+}
+
+
static int
qemuPrepareNVRAM(virQEMUDriver *driver,
virDomainObj *vm,
@@ -4428,13 +4462,8 @@ 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;
if (!loader || !loader->nvram ||
(virFileExists(loader->nvram) && !reset_nvram))
@@ -4458,84 +4487,15 @@ qemuPrepareNVRAM(virQEMUDriver *driver,
goto cleanup;
}
- if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
- 0, -1, -1, 0)) < 0) {
- virReportSystemError(-srcFD,
- _("Failed to open file '%s'"),
- master_nvram_path);
+ if (virFileRewrite(loader->nvram,
+ S_IRUSR | S_IWUSR,
+ cfg->user, cfg->group,
+ qemuPrepareNVRAMHelper,
+ master_nvram_path) < 0)
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);
- 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
On Thu, Feb 10, 2022 at 12:13:26PM +0100, Michal Privoznik wrote:
> 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@redhat.com>
> ---
> src/qemu/qemu_process.c | 118 +++++++++++++---------------------------
> 1 file changed, 39 insertions(+), 79 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8fccf6b760..500a91f153 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4421,6 +4421,40 @@ qemuProcessUpdateCPU(virQEMUDriver *driver,
> }
>
>
> +static int
> +qemuPrepareNVRAMHelper(int dstFD,
> + const void *opaque)
> +{
> + const char *master_nvram_path = opaque;
> + VIR_AUTOCLOSE srcFD = -1;
> + ssize_t r;
> +
> + if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
> + 0, -1, -1, 0)) < 0) {
> + virReportSystemError(-srcFD,
> + _("Failed to open file '%s'"),
> + master_nvram_path);
> + return -2;
> + }
Can't we keep this in the caller and pass 'srcFD' in via opaque ?
> +
> + do {
> + char buf[1024];
> +
> + if ((r = saferead(srcFD, buf, sizeof(buf))) < 0) {
> + virReportSystemError(errno,
> + _("Unable to read from file '%s'"),
> + master_nvram_path);
> + return -2;
> + }
> +
> + if (safewrite(dstFD, buf, r) < 0)
> + return -1;
This just strengthens my view that we should only have 1 return
value for errors and be consistent in usage.
> + } while (r);
> +
> + return 0;
> +}
> +
> +
> static int
> qemuPrepareNVRAM(virQEMUDriver *driver,
> virDomainObj *vm,
> @@ -4428,13 +4462,8 @@ 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;
>
> if (!loader || !loader->nvram ||
> (virFileExists(loader->nvram) && !reset_nvram))
> @@ -4458,84 +4487,15 @@ qemuPrepareNVRAM(virQEMUDriver *driver,
> goto cleanup;
> }
>
> - if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
> - 0, -1, -1, 0)) < 0) {
> - virReportSystemError(-srcFD,
> - _("Failed to open file '%s'"),
> - master_nvram_path);
We don't need to remove this surely, if we pass...
> + if (virFileRewrite(loader->nvram,
> + S_IRUSR | S_IWUSR,
> + cfg->user, cfg->group,
> + qemuPrepareNVRAMHelper,
> + master_nvram_path) < 0)
... &srcFD instead of master_nvram_path here ?
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/10/22 17:16, Daniel P. Berrangé wrote:
> On Thu, Feb 10, 2022 at 12:13:26PM +0100, Michal Privoznik wrote:
>> 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@redhat.com>
>> ---
>> src/qemu/qemu_process.c | 118 +++++++++++++---------------------------
>> 1 file changed, 39 insertions(+), 79 deletions(-)
>>
>> static int
>> qemuPrepareNVRAM(virQEMUDriver *driver,
>> virDomainObj *vm,
>> @@ -4428,13 +4462,8 @@ 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;
>>
>> if (!loader || !loader->nvram ||
>> (virFileExists(loader->nvram) && !reset_nvram))
>> @@ -4458,84 +4487,15 @@ qemuPrepareNVRAM(virQEMUDriver *driver,
>> goto cleanup;
>> }
>>
>> - if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
>> - 0, -1, -1, 0)) < 0) {
>> - virReportSystemError(-srcFD,
>> - _("Failed to open file '%s'"),
>> - master_nvram_path);
>
> We don't need to remove this surely, if we pass...
>
>> + if (virFileRewrite(loader->nvram,
>> + S_IRUSR | S_IWUSR,
>> + cfg->user, cfg->group,
>> + qemuPrepareNVRAMHelper,
>> + master_nvram_path) < 0)
>
> ... &srcFD instead of master_nvram_path here ?
Yep. My aim was to have a self contained callback, but it's just a
matter of preference. Let me fix that in v2.
Michal
© 2016 - 2026 Red Hat, Inc.