Re: [PATCH v2] qga: implement 'guest-get-fsinfo' for FreeBSD

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Jul 03, 2026 at 06:31:07PM +0200, Roman Bogorodskiy wrote:
> Implement the qmp_guest_get_fsinfo() API for FreeBSD.
> To implement it, reuse build_fs_mount_list() which is already
> implemented for FreeBSD. Extend the FsMount type to include
> a string "fromname" attribute to store the disk the FS is mounted from.
> 
> Disk mapping info is not covered by this implementation.
> 
> Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> Acked-by: Markus Armbruster <armbru@redhat.com>
> ---
> Changes since v1:
> 
>  - Replace g_malloc0() with g_new0(.., 1) for GuestFilesystemInfo
> 
> 
>  qga/commands-bsd.c    | 62 ++++++++++++++++++++++++++++++++++++++++++-
>  qga/commands-common.h |  1 +
>  qga/commands-posix.c  |  3 +++
>  qga/qapi-schema.json  | 12 ++++-----
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 

> +#if defined(__FreeBSD__)
> +static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
> +                                               Error **errp)

Nothing ever sets 'errp' so this method cannot fail. Drop this
parameter.

> +{
> +    GuestFilesystemInfo *fs = g_new0(GuestFilesystemInfo, 1);
> +    struct statvfs buf;
> +    unsigned long used, nonroot_total, fr_size;
> +
> +    fs->mountpoint = g_strdup(mount->dirname);
> +    fs->type = g_strdup(mount->devtype);
> +    fs->name = g_strdup(g_path_get_basename(mount->fromname));

Memoory leak - g_path_get_basename already returns a new heap
allocated string, so drop the g_strdup:

  https://docs.gtk.org/glib/func.path_get_basename.html

> +
> +    if (statvfs(fs->mountpoint, &buf) == 0) {
> +        fr_size = buf.f_frsize;
> +        used = buf.f_blocks - buf.f_bfree;
> +        nonroot_total = used + buf.f_bavail;
> +        fs->used_bytes = used * fr_size;
> +        fs->total_bytes = nonroot_total * fr_size;
> +        fs->total_bytes_privileged = buf.f_blocks * fr_size;
> +
> +        fs->has_total_bytes = true;
> +        fs->has_total_bytes_privileged = true;
> +        fs->has_used_bytes = true;
> +    }
> +
> +    return fs;
> +}
> +
> +GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
> +{
> +    FsMountList mounts;
> +    struct FsMount *mount;
> +    GuestFilesystemInfoList *ret = NULL;
> +    Error *local_err = NULL;
> +
> +    QTAILQ_INIT(&mounts);
> +    if (!build_fs_mount_list(&mounts, &local_err)) {
> +        error_propagate(errp, local_err);

Pointless use of local_err - pass errp and drop error_propagate

> +        return NULL;
> +    }
> +
> +    QTAILQ_FOREACH(mount, &mounts, next) {
> +        g_debug("Building guest fsinfo for '%s'", mount->dirname);
> +
> +        QAPI_LIST_PREPEND(ret, build_guest_fsinfo(mount, &local_err));
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            qapi_free_GuestFilesystemInfoList(ret);
> +            ret = NULL;
> +            break;
> +        }

build_guest_fsinfo cannot fail so this branch is dead code, which
means local_err can be entirely dropped.

> +    }
> +
> +    free_fs_mount_list(&mounts);
> +    return ret;
> +}
> +#endif /* __FreeBSD__ */

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|