[PATCH 0/5] More reset nvram fixes

[PATCH 0/6] ch_driver: Misc...

removing VMs logs

Michal Privoznik

Thursday, 10 February 2022 Thu, 10 Feb '22

5:13 a.m.

I've played with the new functionality and found couple of bugs/possible improvements. Michal Prívozník (5): qemu_process: Accept VIR_QEMU_PROCESS_START_RESET_NVRAM flag when starting a domain qemuFirmwareFillDomain: Use FW descriptors to lookup template when resetting NVRAM virFileRewrite: Allow setting owner virFileRewrite: Allow callback report errors qemuPrepareNVRAM: Us virFileRewrite() to write NVRAM src/qemu/qemu_firmware.c | 3 +- src/qemu/qemu_process.c | 124 ++++++++++++++------------------------- src/util/virfile.c | 41 +++++++++++-- src/util/virfile.h | 1 + src/util/virxml.c | 3 +- 5 files changed, 83 insertions(+), 89 deletions(-) -- 2.34.1

Show replies by date

Michal Privoznik

Thursday, 10 February Thu, 10 Feb

5:13 a.m.

New subject: [PATCH 1/5] qemu_process: Accept VIR_QEMU_PROCESS_START_RESET_NVRAM flag when starting a domain

In one of my previous commits I've fixed the value of VIR_QEMU_PROCESS_START_RESET_NVRAM flag (which was masking another value). But what I forgot to do is update virCheckFlags() calls in two places where the flag is passed: qemuProcessLaunch() and qemuProcessStart(). Fixes: 1b636593c76f443169ef7bdb3644fd670379d04e Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 24873f6fb7..8fccf6b760 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7380,7 +7380,8 @@ qemuProcessLaunch(virConnectPtr conn, VIR_QEMU_PROCESS_START_PAUSED | VIR_QEMU_PROCESS_START_AUTODESTROY | VIR_QEMU_PROCESS_START_NEW | - VIR_QEMU_PROCESS_START_GEN_VMID, -1); + VIR_QEMU_PROCESS_START_GEN_VMID | + VIR_QEMU_PROCESS_START_RESET_NVRAM, -1); cfg = virQEMUDriverGetConfig(driver); @@ -7807,7 +7808,8 @@ qemuProcessStart(virConnectPtr conn, virCheckFlagsGoto(VIR_QEMU_PROCESS_START_COLD | VIR_QEMU_PROCESS_START_PAUSED | VIR_QEMU_PROCESS_START_AUTODESTROY | - VIR_QEMU_PROCESS_START_GEN_VMID, cleanup); + VIR_QEMU_PROCESS_START_GEN_VMID | + VIR_QEMU_PROCESS_START_RESET_NVRAM, cleanup); if (!migrateFrom && !snapshot) flags |= VIR_QEMU_PROCESS_START_NEW; -- 2.34.1

Daniel P. Berrangé

10:04 a.m.

New subject: [PATCH 1/5] qemu_process: Accept VIR_QEMU_PROCESS_START_RESET_NVRAM flag when starting a domain

On Thu, Feb 10, 2022 at 12:13:22PM +0100, Michal Privoznik wrote:

...

Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com> 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 :|

Michal Privoznik

5:13 a.m.

New subject: [PATCH 2/5] qemuFirmwareFillDomain: Use FW descriptors to lookup template when resetting NVRAM

If VIR_QEMU_PROCESS_START_RESET_NVRAM flag is passed when starting a domain, then user requested to overwrite the domain specific NVRAM with the one from template. But it is very likely that the path to the template is not stored in the domain definition, which in turn makes the copy function (qemuPrepareNVRAM()) fail. The solution is simple - when preparing domain, specifically when deciding whether the path to the template should be autofilled, ignore any existing NVRAM file. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/qemu/qemu_firmware.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 84c80eaacb..ff364996b8 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1297,6 +1297,7 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, ssize_t nfirmwares = 0; const qemuFirmware *theone = NULL; bool needResult = true; + const bool reset_nvram = flags & VIR_QEMU_PROCESS_START_RESET_NVRAM; size_t i; int ret = -1; @@ -1309,7 +1310,7 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, * specified and the varstore doesn't exist ... */ if (!virDomainDefHasOldStyleROUEFI(def) || def->os.loader->templt || - virFileExists(def->os.loader->nvram)) + (!reset_nvram && virFileExists(def->os.loader->nvram))) return 0; /* ... then we want to consult JSON FW descriptors first, -- 2.34.1

Daniel P. Berrangé

10:05 a.m.

New subject: [PATCH 2/5] qemuFirmwareFillDomain: Use FW descriptors to lookup template when resetting NVRAM

On Thu, Feb 10, 2022 at 12:13:23PM +0100, Michal Privoznik wrote:

...

Michal Privoznik

5:13 a.m.

New subject: [PATCH 3/5] virFileRewrite: Allow setting owner

Currently, due to the way virFileRewrite() works, the rewritten file is owned by user and group that the daemon runs under. So far, this is not a problem, because the function is used to write XML files or secrets for persistent objects (domains, networks, etc.) and we don't need other users to read/write those files. But shortly, this function is going to be used for creating files for QEMU domains. There we want the QEMU process (i.e. different user) to read the file. Therefore, introduce two new arguments: @uid and @gid that allow setting desired owner of the file. Pass -1 to preserve current behaviour (i.e. create the file owned by the user running the daemon). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/util/virfile.c | 28 +++++++++++++++++++++++++--- src/util/virfile.h | 1 + src/util/virxml.c | 3 ++- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 0b79772da7..f99e7f95e1 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -484,9 +484,28 @@ int virFileUnlock(int fd G_GNUC_UNUSED, #endif /* WIN32 */ +/** + * virFileRewrite: + * @path: file to rewrite + * @mode: mode of the file + * @uid: uid that should own file + * @gid: gid that should own file + * @rewrite: callback to write file contents + * @opaque: opaque data to pass to the callback + * + * Rewrite given @path atomically. This is achieved by writing a + * 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. + * + * Returns: 0 on success, + * -1 otherwise (with error reported) + */ int virFileRewrite(const char *path, mode_t mode, + uid_t uid, gid_t gid, virFileRewriteFunc rewrite, const void *opaque) { @@ -496,8 +515,11 @@ virFileRewrite(const char *path, newfile = g_strdup_printf("%s.new", path); - if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, mode)) < 0) { - virReportSystemError(errno, _("cannot create file '%s'"), + if ((fd = virFileOpenAs(newfile, O_WRONLY | O_CREAT | O_TRUNC, mode, + uid, gid, + VIR_FILE_OPEN_FORCE_OWNER | VIR_FILE_OPEN_FORCE_MODE)) < 0) { + virReportSystemError(-fd, + _("Failed to create file '%s'"), newfile); goto cleanup; } @@ -552,7 +574,7 @@ virFileRewriteStr(const char *path, mode_t mode, const char *str) { - return virFileRewrite(path, mode, + return virFileRewrite(path, mode, -1, -1, virFileRewriteStrHelper, str); } diff --git a/src/util/virfile.h b/src/util/virfile.h index 967c9a9b4f..34184b32aa 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -126,6 +126,7 @@ int virFileUnlock(int fd, off_t start, off_t len) typedef int (*virFileRewriteFunc)(int fd, const void *opaque); int virFileRewrite(const char *path, mode_t mode, + uid_t uid, gid_t gid, virFileRewriteFunc rewrite, const void *opaque); int virFileRewriteStr(const char *path, diff --git a/src/util/virxml.c b/src/util/virxml.c index bb1ae3e305..a55eb9629b 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1195,7 +1195,8 @@ virXMLSaveFile(const char *path, { struct virXMLRewriteFileData data = { warnName, warnCommand, xml }; - return virFileRewrite(path, S_IRUSR | S_IWUSR, virXMLRewriteFile, &data); + return virFileRewrite(path, S_IRUSR | S_IWUSR, -1, -1, + virXMLRewriteFile, &data); } /** -- 2.34.1

Daniel P. Berrangé

10:06 a.m.

New subject: [PATCH 3/5] virFileRewrite: Allow setting owner

On Thu, Feb 10, 2022 at 12:13:24PM +0100, Michal Privoznik wrote:

...

Michal Privoznik

5:13 a.m.

New subject: [PATCH 4/5] virFileRewrite: Allow callback report errors

Sometimes it may be handy for the callback to report error, even though our current callbacks are trivial. Let's report an error only if callback returns a well known value, otherwise assume it reported error message on its own. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/util/virfile.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index f99e7f95e1..dd065a537c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -499,6 +499,10 @@ int virFileUnlock(int fd G_GNUC_UNUSED, * @uid:@gid (pass -1 for current uid/gid) and written by * @rewrite callback. * + * A negative value returned by @rewrite callback is treated as + * error and if the value is different to -1 then it's the + * callback's responsibility to report error. + * * Returns: 0 on success, * -1 otherwise (with error reported) */ @@ -512,6 +516,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 +529,11 @@ virFileRewrite(const char *path, goto cleanup; } - if (rewrite(fd, opaque) < 0) { - virReportSystemError(errno, _("cannot write data to file '%s'"), - newfile); + if ((rc = rewrite(fd, opaque)) < 0) { + if (rc == -1) { + virReportSystemError(errno, _("cannot write data to file '%s'"), + newfile); + } goto cleanup; } -- 2.34.1

Daniel P. Berrangé

10:12 a.m.

New subject: [PATCH 4/5] virFileRewrite: Allow callback report errors

On Thu, Feb 10, 2022 at 12:13:25PM +0100, Michal Privoznik wrote:

...

I'd sugest just updating the existing caller to always report an error, rather than having different semantics for -1 vs -2.

...

@@ -524,9 +529,11 @@ virFileRewrite(const char *path, goto cleanup; } - if (rewrite(fd, opaque) < 0) { - virReportSystemError(errno, _("cannot write data to file '%s'"), - newfile);

And simply get rid of this virReportSystemError call entirely

...

+ if ((rc = rewrite(fd, opaque)) < 0) { + if (rc == -1) { + virReportSystemError(errno, _("cannot write data to file '%s'"), + newfile); + } goto cleanup; } -- 2.34.1

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 :|

Michal Prívozník

Friday, 11 February Fri, 11 Feb

2:03 a.m.

New subject: [PATCH 4/5] virFileRewrite: Allow callback report errors

On 2/10/22 17:12, Daniel P. Berrangé wrote:

...

On Thu, Feb 10, 2022 at 12:13:25PM +0100, Michal Privoznik wrote: > Sometimes it may be handy for the callback to report error, even > though our current callbacks are trivial. Let's report an error > only if callback returns a well known value, otherwise assume it > reported error message on its own. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> > --- > src/util/virfile.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/src/util/virfile.c b/src/util/virfile.c > index f99e7f95e1..dd065a537c 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.c > @@ -499,6 +499,10 @@ int virFileUnlock(int fd G_GNUC_UNUSED, > * @uid:@gid (pass -1 for current uid/gid) and written by > * @rewrite callback. > * > + * A negative value returned by @rewrite callback is treated as > + * error and if the value is different to -1 then it's the > + * callback's responsibility to report error. I'd sugest just updating the existing caller to always report an error, rather than having different semantics for -1 vs -2. > @@ -524,9 +529,11 @@ virFileRewrite(const char *path, > goto cleanup; > } > > - if (rewrite(fd, opaque) < 0) { > - virReportSystemError(errno, _("cannot write data to file '%s'"), > - newfile); And simply get rid of this virReportSystemError call entirely

Fair enough, I wanted to not change the signature of the callback, because the filename is not passed into it. But that's fairly trivial change. Michal

Michal Privoznik

Thursday, 10 February Thu, 10 Feb

5:13 a.m.

New subject: [PATCH 5/5] qemuPrepareNVRAM: Us virFileRewrite() to write NVRAM

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> --- 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

Daniel P. Berrangé

10:16 a.m.

New subject: [PATCH 5/5] qemuPrepareNVRAM: Us virFileRewrite() to write NVRAM

On Thu, Feb 10, 2022 at 12:13:26PM +0100, Michal Privoznik wrote:

...

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 :|

Michal Prívozník

Friday, 11 February Fri, 11 Feb

2:02 a.m.

New subject: [PATCH 5/5] qemuPrepareNVRAM: Us virFileRewrite() to write NVRAM

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(a)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

1015

days inactive

1016

days old

devel@lists.libvirt.org

Manage subscription

12 comments

3 participants

tags (0)

participants (3)

Daniel P. Berrangé
Michal Privoznik
Michal Prívozník

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[PATCH 0/5] More reset nvram fixes