[libvirt PATCH] storage_backend_fs: use MKFS ony if WITH_STORAGE_FS is defined
Michal Privoznik
mprivozn at redhat.com
Wed Apr 21 14:43:01 UTC 2021
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().
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
More information about the libvir-list
mailing list