1. Patchew
  2. Libvirt
  3. View series

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

Pavel Hrdina posted 1 patch 3 years ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/af3a304eb25f5eb2d0995875f43818c0c7aa7d79.1619011967.git.phrdina@redhat.com
There is a newer version of this series
src/storage/storage_backend_fs.c | 2 ++
1 file changed, 2 insertions(+)
[libvirt PATCH] storage_backend_fs: use MKFS ony if WITH_STORAGE_FS is defined
Posted by Pavel Hrdina 3 years ago 
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@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;
 }
 
-- 
2.30.2
Re: [libvirt PATCH] storage_backend_fs: use MKFS ony if WITH_STORAGE_FS is defined
Posted by Michal Privoznik 3 years ago 
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@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

      if (!mkfs) {
          virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -412,7 +416,7 @@ virStorageBackendExecuteMKFS(const char *device,
          return -1;
      }

-    cmd = virCommandNewArgList(MKFS, "-t", format, NULL);
+    cmd = virCommandNewArgList(mkfs, "-t", format, NULL);

      /* use the force, otherwise mkfs.xfs won't overwrite existing fs.
       * Similarly mkfs.ext2, mkfs.ext3, and mkfs.ext4 require supplying -F

Michal
Re: [libvirt PATCH] storage_backend_fs: use MKFS ony if WITH_STORAGE_FS is defined
Posted by Pavel Hrdina 3 years ago 
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@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.

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

Pavel

[1] <https://listman.redhat.com/archives/libvir-list/2021-April/msg00973.html>
Re: [libvirt PATCH] storage_backend_fs: use MKFS ony if WITH_STORAGE_FS is defined
Posted by Michal Privoznik 3 years ago 
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@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().

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

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.