Re: [PATCH] qga: skip bind mounts in fs list

[Jean-Louis Dupond <jean-louis@dupond.be>]

On 9/10/2024 10:34, Jean-Louis Dupond wrote:
> On 2/10/2024 12:06, Jean-Louis Dupond wrote:
>> The filesystem list in build_fs_mount_list should skip bind mounts.
>> This because we end up in locking situations when doing fsFreeze. Like
>> mentioned in [1] and [2].
>>
>> Next to that, the build_fs_mount_list call did a fallback via
>> build_fs_mount_list_from_mtab if mountinfo did not exist.
>> There it skipped bind mounts, but this is broken for newer OS.
>> This as mounts does not return the path of the bind mount but the
>> underlying dev/partition, so S_ISDIR will never return true in
>> dev_major_minor call.
>>
>> This patch simply checks the existing devmajor:devminor tuple in the
>> mounts, and if it already exists, this means we have the same devices
>> mounted again, a bind mount. So skip this.
>>
>> Same approach is used in open-vm-tools [3].
>>
>> [1]: https://gitlab.com/qemu-project/qemu/-/issues/592
>> [2]: https://gitlab.com/qemu-project/qemu/-/issues/520
>> [3]: 
>> https://github.com/vmware/open-vm-tools/commit/d58847b497e212737007958c945af1df22a8ab58
>>
>> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
>> ---
>>   qga/commands-linux.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
>> index 51d5e3d927..426b040ab8 100644
>> --- a/qga/commands-linux.c
>> +++ b/qga/commands-linux.c
>> @@ -59,6 +59,22 @@ static int dev_major_minor(const char *devpath,
>>       return -1;
>>   }
>>   +/*
>> + * Check if we already have the devmajor:devminor in the mounts
>> + * If thats the case return true.
>> + */
>> +static bool dev_exists(FsMountList *mounts, unsigned int devmajor, 
>> unsigned int devminor)
>> +{
>> +    FsMount *mount;
>> +
>> +    QTAILQ_FOREACH(mount, mounts, next) {
>> +        if (mount->devmajor == devmajor && mount->devminor == 
>> devminor) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +
>>   static bool build_fs_mount_list_from_mtab(FsMountList *mounts, 
>> Error **errp)
>>   {
>>       struct mntent *ment;
>> @@ -89,6 +105,10 @@ static bool 
>> build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
>>               /* Skip bind mounts */
>>               continue;
>>           }
>> +        if (dev_exists(mounts, devmajor, devminor)) {
>> +            /* Skip already existing devices (bind mounts) */
>> +            continue;
>> +        }
>>             mount = g_new0(FsMount, 1);
>>           mount->dirname = g_strdup(ment->mnt_dir);
>> @@ -172,6 +192,11 @@ bool build_fs_mount_list(FsMountList *mounts, 
>> Error **errp)
>>               }
>>           }
>>   +        if (dev_exists(mounts, devmajor, devminor)) {
>> +            /* Skip already existing devices (bind mounts) */
>> +            continue;
>> +        }
>> +
>>           mount = g_new0(FsMount, 1);
>>           mount->dirname = g_strdup(line + dir_s);
>>           mount->devtype = g_strdup(dash + type_s);
>
>
> Ping + add kkostiuk@redhat.com as I missed him in the initial mail.
>

Any chance on a review or getting it merged?
Think it's a good (of course ;)) improvement.