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

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(a)redhat.com&gt; > --- > 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.

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(a)redhat.com&gt;