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

[Pavel Hrdina]

On Wed, Apr 21, 2021 at 04:43:01PM +0200, Michal Privoznik wrote:
> 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 at redhat.com>
> > > > ---
> > > > 
> > > > 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().

That's unrelated issue we should address as well. In meson we enable
storage_dir without checking for any dependnecies, the 'mkfs' binary
is checked only for storage_fs. My guess is that it was the same with
autotools as well. In addition I don't see why mkfs should be used by
storage_dir as it operates only with directories.

One way how 'mkfs' can be executed is using

    virsh pool-build $name --overwrit

but doing so on DIR pool resulted in the following error:

error: Failed to build pool test
error: Requested operation is not valid: No source device specified when formatting pool 'test'

So as I suspected it will not be used at all. So the code should be
fixed in a way that we would not even try to call the functions leading
to the 'mkfs' call.

> 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 :-)

I'll post a v2 to fix the build issue and we the actual problem can be
fixed later.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210421/6ed502f6/attachment-0001.sig>