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

On 4/21/21 4:23 PM, Pavel Hrdina wrote: > 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. But MKFS is not only used when WITH_STORAGE_FS. It's also used for VIR_STORAGE_POOL_DIR: There' a path from virStorageBackendFileSystemBuild() to virStorageBackendExecuteMKFS().

And your suggested patches are not merged yet, thus what I'm suggesting feels a bit more correct. > > If you are OK with using WITH_STORAGE_FS instead of MKFS in the ifdef > we can push this version. But okay, I guess I can live with success reported incorrectly on freebsd and mac :-)