Re: [libvirt PATCH] storage_backend_fs: use MKFS ony if WITH_STORAGE_FS is defined

On Wed, Apr 21, 2021 at 04:06:05PM +0200, Michal Privoznik wrote: > On 4/21/21 3:33 PM, Pavel Hrdina wrote: >> The code in storage_backend_fs is used for storage_dir and storage_fs >> drivers so some parts need to be guarded by checking for >> WITH_STORAGE_FS. >> >> Fixes: 16c69e7aaed4cabfd8e8c19cc326854d4c480437 >> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; >> --- >> >> GitLab CI faild on MacOS and FreeBSD where storage_fs is disabled. >> >> src/storage/storage_backend_fs.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c >> index af645ea9de..e6a348521d 100644 >> --- a/src/storage/storage_backend_fs.c >> +++ b/src/storage/storage_backend_fs.c >> @@ -401,6 +401,7 @@ static int >> virStorageBackendExecuteMKFS(const char *device, >> const char *format) >> { >> +#if WITH_STORAGE_FS >> g_autoptr(virCommand) cmd = NULL; >> g_autofree char *mkfs = virFindFileInPath(MKFS); >> @@ -431,6 +432,7 @@ virStorageBackendExecuteMKFS(const char *device, >> if (virCommandRun(cmd, NULL) < 0) >> return -1; >> +#endif /* WITH_STORAGE_FS */ >> return 0; >> } >> > > This function has only one caller so we can be sure for now that it is not > called when !WITH_STORAGE_FS but I think we would be much more safe if in > that case an error would be returned, instead of return 0. > > Can't we do something like this? > > diff --git c/src/storage/storage_backend_fs.c > w/src/storage/storage_backend_fs.c > index af645ea9de..09da8ec8b6 100644 > --- c/src/storage/storage_backend_fs.c > +++ w/src/storage/storage_backend_fs.c > @@ -402,7 +402,11 @@ virStorageBackendExecuteMKFS(const char *device, > const char *format) > { > g_autoptr(virCommand) cmd = NULL; > - g_autofree char *mkfs = virFindFileInPath(MKFS); > + g_autofree char *mkfs = NULL; > + > +#ifdef MKFS > + mkfs = virFindFileInPath(MKFS); > +#endif I wanted to avoid using #ifdef MKFS because once this series [1] is merged it will be always true. In addition using WITH_STORAGE_FS is what we do for other functions in this file as well so I wanted to keep it consistent.

If you are OK with using WITH_STORAGE_FS instead of MKFS in the ifdef we can push this version.