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

[Michal Prívozník]

On 5/13/22 14:57, Claudio Fontana wrote:
> 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 at redhat.com>
>> ---
>>  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])

That's likely caused by virsh. I haven't checked your patches yet, but
depending on how the argument is queried for in virsh you may get a NULL
or an empty string. However, empty string doesn't matter as it produces
sensible error message:

  No such file or directory: ''

Michal