[libvirt] [PATCH v1 02/15] qemu_domain: Separate NVRAM VAR store file name generation

[Michal Privoznik]

On 2/28/19 10:11 AM, Laszlo Ersek wrote:
> On 02/27/19 11:04, Michal Privoznik wrote:
>> Move the code that (possibly) generates filename of NVRAM VAR
>> store into a single function so that it can be re-used later.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/qemu/qemu_domain.c | 26 ++++++++++++++++++--------
>>   src/qemu/qemu_domain.h |  4 ++++
>>   2 files changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 59fe1eb401..cf7e650b34 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -3876,14 +3876,8 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>>           goto cleanup;
>>       }
>>   
>> -    if (def->os.loader &&
>> -        def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
>> -        def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
>> -        !def->os.loader->nvram) {
>> -        if (virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd",
>> -                        cfg->nvramDir, def->name) < 0)
>> -            goto cleanup;
>> -    }
>> +    if (qemuDomainNVRAMPathGenerate(cfg, def) < 0)
>> +        goto cleanup;
>>   
>>       if (qemuDomainDefAddDefaultDevices(def, qemuCaps) < 0)
>>           goto cleanup;
>> @@ -13944,3 +13938,19 @@ qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk)
>>              virStorageSourceIsLocalStorage(disk->src) && disk->src->path &&
>>              !virFileExists(disk->src->path);
>>   }
>> +
>> +
>> +int
>> +qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg,
>> +                            virDomainDefPtr def)
>> +{
>> +    if (def->os.loader &&
>> +        def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
>> +        def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
>> +        !def->os.loader->nvram) {
>> +        return virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd",
>> +                           cfg->nvramDir, def->name);
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index 7c6b50184c..136a7a7c72 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -1107,4 +1107,8 @@ qemuDomainIsUsingNoShutdown(qemuDomainObjPrivatePtr priv);
>>   bool
>>   qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk);
>>   
>> +int
>> +qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg,
>> +                            virDomainDefPtr def);
>> +
>>   #endif /* LIBVIRT_QEMU_DOMAIN_H */
>>
> 
> I'm not familiar with restrictions on helper functions (e.g. if they are
> supposed to sanity check input params for null pointers etc), but as a
> normal code extraction patch, this looks OK to me.

There are no rules in libvirt for that. Usually, we take the path of 
least resistance. So when some piece of code needs to be reused, we just 
move it into a function and expose it. Without us adding special checks 
(e.g. argument sanitization). Those are added if the function might be 
called from another place where the assumption that arguments are sane 
can't be made. But it's not the case for this function.

> 
> Also presumably the other call will arrive from a different C file,
> hence the external linkage and header file change.
> 
> Reviewed-by: Laszlo Ersek <lersek at redhat.com>
> 

Thanks,
Michal