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

[Roman Bogorodskiy <bogorodskiy@gmail.com>]

  Kostiantyn Kostiuk wrote:

> Hi Roman,
> 
> Thanks for your patch, and sorry for the late reply
> 

Thanks for review!

> On Sun, Jul 19, 2026 at 7:04 PM Roman Bogorodskiy <bogorodskiy@gmail.com>
> wrote:
> 
> > Implement the 'guest-get-diskstats' for FreeBSD.
> > This implementation uses the devstat(3) library which is a part
> > of the FreeBSD base system.
> >
> > The build system is updated to link qga with `-ldevstat`.
> >
> > Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> > ---
> >  qga/commands-bsd.c   | 127 +++++++++++++++++++++++++++++++++++++++++++
> >  qga/meson.build      |   3 +
> >  qga/qapi-schema.json |   6 +-
> >  3 files changed, 133 insertions(+), 3 deletions(-)
> >
> > diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
> > index 94ff6fee6a..763db33c9f 100644
> > --- a/qga/commands-bsd.c
> > +++ b/qga/commands-bsd.c
> > @@ -27,6 +27,9 @@
> >  #include <net/ethernet.h>
> >  #endif
> >  #include <paths.h>
> > +#ifdef CONFIG_FREEBSD
> > +#include <devstat.h>
> > +#endif
> >
> >  #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
> >  bool build_fs_mount_list(FsMountList *mounts, Error **errp)
> > @@ -178,3 +181,127 @@ bool guest_get_hw_addr(struct ifaddrs *ifa, unsigned
> > char *buf,
> >      return true;
> >  }
> >  #endif /* HAVE_GETIFADDRS */
> > +
> > +#ifdef CONFIG_FREEBSD
> > +static uint64_t bintime_to_msec(const struct bintime *bt)
> > +{
> > +    return (uint64_t)bt->sec * 1000ULL + (((bt->frac >> 32) * 1000ULL) >>
> > 32);
> > +}
> > +
> > +static void guest_diskstats_append(GuestDiskStatsInfoList ***tailp,
> > +                                   const struct devstat *dev)
> >
> 
> Can we simplify pointer logic?
> For example, return GuestDiskStatsInfo* for devstat* and then append the
> list in guest_get_diskstats.
> ***tailp -> more pointers - easier to make a mistake or memory leak.

Yes, will address in v2.

> > +
> > +    diskstat->has_ios_pgr = true;
> > +    if (dev->start_count >= dev->end_count) {
> > +        diskstat->ios_pgr = dev->start_count - dev->end_count;
> > +    }
> >
> 
> In case start_count < end_count, you have has_ios_pgr = true, and ios_pgr =
> 0 (g_new0 initialized to 0’s)
> Is this expected? And in general, is this possible?

That's an interesting question.
`start_count` and `end_count` are uint's. It is possible that
`start_count` will wrap sooner than `end_count`, so 

   start_count < end_count

But apparently the correct behaviour is still report the difference
instead of reporting 0.

devstat(9) describes that:

     start_count        Number of operations started.

     end_count          Number of operations completed.  The “busy_count” can
                        be calculated by subtracting end_count from
                        start_count.  (sequence0 and sequence1 are used to get
                        a consistent snapshot.)  This is the current number of
                        outstanding transactions for the device.  This should
                        never go below zero, and on an idle device it should
                        be zero.  If either one of these conditions is not
                        true, it indicates a problem.


So it looks like the conditional is not needed. libdevstat in FreeBSD
does not have it too [1].
I'll remove that in v2.

> > +
> > +GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
> > +{
> > +    return guest_get_diskstats(errp);
> >
> 
> Any reason for this wrapper?
> I see it in the Linux implementation, but not for other commands.

I followed the Linux implementation schema here. Apparently it is not
necessary. Will address in v2 as well.

1: https://cgit.freebsd.org/src/tree/lib/libdevstat/devstat.c#n1547