[libvirt PATCH] storage_backend_fs: use MKFS ony if WITH_STORAGE_FS is defined
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
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
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>
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
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@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
participants (2)
-
Michal Privoznik -
Pavel Hrdina