Re: [PATCH v2 2/3] lib: Repurpose virDomainSaveParams() with no VIR_DOMAIN_SAVE_PARAM_FILE

On 5/13/22 9:54 AM, Michal Privoznik wrote: > When no VIR_DOMAIN_SAVE_PARAM_FILE typed param is set when > calling virDomainSaveParams() then in turn virQEMUFileOpenAs() > tries to open a NULL path. > > We have two options now: > 1) require the typed param, which in turn may be promoted to a > regular argument, or > > 2) use this opportunity to make the API behave like > virDomainManagedSave() and use typed params to pass extra > arguments, instead of having to invent new managed save API > with typed params. > > Let's go with option 2, as it is more future proof. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/libvirt-domain.c | 2 ++ > src/qemu/qemu_driver.c | 33 ++++++++++++++++++++------------- > 2 files changed, 22 insertions(+), 13 deletions(-) > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > index d1d62daa71..208c2438fe 100644 > --- a/src/libvirt-domain.c > +++ b/src/libvirt-domain.c > @@ -1007,6 +1007,8 @@ virDomainSaveFlags(virDomainPtr domain, const char *to, > * @flags: bitwise-OR of virDomainSaveRestoreFlags > * > * This method extends virDomainSaveFlags by adding parameters. > + * If VIR_DOMAIN_SAVE_PARAM_FILE is not provided then a managed save is > + * performed (see virDomainManagedSave). > * > * Returns 0 in case of success and -1 in case of failure. > * > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 7d8c7176d9..0b31c73bb9 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -2772,6 +2772,7 @@ qemuDomainManagedSavePath(virQEMUDriver *driver, virDomainObj *vm) > static int > qemuDomainManagedSaveHelper(virQEMUDriver *driver, > virDomainObj *vm, > + const char *dxml, > unsigned int flags) > { > g_autoptr(virQEMUDriverConfig) cfg = NULL; > @@ -2799,7 +2800,7 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver, > VIR_INFO("Saving state of domain '%s' to '%s'", vm->def->name, path); > > if (qemuDomainSaveInternal(driver, vm, path, compressed, > - compressor, NULL, flags) < 0) > + compressor, dxml, flags) < 0) > return -1; > > vm->hasManagedSave = true; > @@ -2853,17 +2854,18 @@ qemuDomainSave(virDomainPtr dom, const char *path) > > static int > qemuDomainSaveParams(virDomainPtr dom, > - virTypedParameterPtr params, int nparams, > + virTypedParameterPtr params, > + int nparams, > unsigned int flags) > { > + virQEMUDriver *driver = dom->conn->privateData; > + g_autoptr(virQEMUDriverConfig) cfg = NULL; > + virDomainObj *vm = NULL; > + g_autoptr(virCommand) compressor = NULL; > const char *to = NULL; > const char *dxml = NULL; > - virQEMUDriver *driver = dom->conn->privateData; > int compressed; > - g_autoptr(virCommand) compressor = NULL; > int ret = -1; > - virDomainObj *vm = NULL; > - g_autoptr(virQEMUDriverConfig) cfg = NULL; > > virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | > VIR_DOMAIN_SAVE_RUNNING | > @@ -2884,18 +2886,23 @@ qemuDomainSaveParams(virDomainPtr dom, > VIR_DOMAIN_SAVE_PARAM_DXML, &dxml) < 0) > return -1; > > - cfg = virQEMUDriverGetConfig(driver); > - if ((compressed = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, > - &compressor, > - "save", false)) < 0) > - goto cleanup; > - > if (!(vm = qemuDomainObjFromDomain(dom))) > goto cleanup; > > if (virDomainSaveParamsEnsureACL(dom->conn, vm->def) < 0) > goto cleanup; > > + if (!to) { > + /* If no save path was provided then this behaves as managed save. */ > + return qemuDomainManagedSaveHelper(driver, vm, dxml, flags); > + } Is this sufficient? In some cases I am seeing that omitted string parameters are passed as the empty string. In my multifd series I encountered this with the optional --parallel-compression argument, that in this case would mean: if (!to || !to[0])