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

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

[Roman Bogorodskiy]

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)
+{
+    GuestDiskStatsInfoList **tail = *tailp;
+    g_autofree GuestDiskStatsInfo *diskstatinfo = NULL;
+    g_autofree GuestDiskStats *diskstat = NULL;
+
+    diskstatinfo = g_new0(GuestDiskStatsInfo, 1);
+    diskstatinfo->name = g_strdup_printf("%s%d", dev->device_name,
+                                         dev->unit_number);
+    /*
+     * devstat does not expose Linux-style major/minor numbers.  Report the
+     * devstat device number and unit number in these mandatory QAPI fields.
+     */
+    diskstatinfo->major = dev->device_number;
+    diskstatinfo->minor = dev->unit_number;
+
+    diskstat = g_new0(GuestDiskStats, 1);
+    diskstat->has_read_ios = true;
+    diskstat->read_ios = dev->operations[DEVSTAT_READ];
+    diskstat->has_read_sectors = true;
+    diskstat->read_sectors = dev->bytes[DEVSTAT_READ] / BDRV_SECTOR_SIZE;
+    diskstat->has_read_ticks = true;
+    diskstat->read_ticks = bintime_to_msec(&dev->duration[DEVSTAT_READ]);
+
+    diskstat->has_write_ios = true;
+    diskstat->write_ios = dev->operations[DEVSTAT_WRITE];
+    diskstat->has_write_sectors = true;
+    diskstat->write_sectors = dev->bytes[DEVSTAT_WRITE] / BDRV_SECTOR_SIZE;
+    diskstat->has_write_ticks = true;
+    diskstat->write_ticks = bintime_to_msec(&dev->duration[DEVSTAT_WRITE]);
+
+    diskstat->has_discard_ios = true;
+    diskstat->discard_ios = dev->operations[DEVSTAT_FREE];
+    diskstat->has_discard_sectors = true;
+    diskstat->discard_sectors = dev->bytes[DEVSTAT_FREE] / BDRV_SECTOR_SIZE;
+    diskstat->has_discard_ticks = true;
+    diskstat->discard_ticks = bintime_to_msec(&dev->duration[DEVSTAT_FREE]);
+
+    diskstat->has_ios_pgr = true;
+    if (dev->start_count >= dev->end_count) {
+        diskstat->ios_pgr = dev->start_count - dev->end_count;
+    }
+
+    diskstat->has_total_ticks = true;
+    diskstat->total_ticks = bintime_to_msec(&dev->busy_time);
+
+    diskstatinfo->stats = g_steal_pointer(&diskstat);
+    QAPI_LIST_APPEND(tail, diskstatinfo);
+    diskstatinfo = NULL;
+
+    *tailp = tail;
+}
+
+static GuestDiskStatsInfoList *guest_get_diskstats(Error **errp)
+{
+    GuestDiskStatsInfoList *head = NULL, **tail = &head;
+    struct devinfo dinfo = { 0 };
+    struct statinfo stats = { .dinfo = &dinfo };
+    struct device_selection *dev_select = NULL;
+    struct devstat_match matches[] = {
+        {
+            .match_fields = DEVSTAT_MATCH_TYPE,
+            .device_type = DEVSTAT_TYPE_DIRECT,
+            .num_match_categories = 1,
+        },
+    };
+    int num_selected = 0;
+    int num_selections = 0;
+    long select_generation = 0;
+    int i;
+
+    if (devstat_checkversion(NULL) == -1) {
+        error_setg(errp, "%s", devstat_errbuf);
+        return NULL;
+    }
+
+    if (devstat_getdevs(NULL, &stats) == -1) {
+        error_setg(errp, "%s", devstat_errbuf);
+        return NULL;
+    }
+
+    if (devstat_selectdevs(&dev_select, &num_selected, &num_selections,
+                           &select_generation, stats.dinfo->generation,
+                           stats.dinfo->devices, stats.dinfo->numdevs,
+                           matches, ARRAY_SIZE(matches), NULL, 0,
+                           DS_SELECT_ONLY,
+                           stats.dinfo->numdevs, 0) == -1) {
+        error_setg(errp, "%s", devstat_errbuf);
+        goto error;
+    }
+
+    for (i = 0; i < num_selections; i++) {
+        if (dev_select[i].selected == 0) {
+            continue;
+        }
+
+        guest_diskstats_append(&tail,
+                               &stats.dinfo->devices[dev_select[i].position]);
+    }
+
+    free(stats.dinfo->mem_ptr);
+    free(dev_select);
+    return head;
+
+error:
+    qapi_free_GuestDiskStatsInfoList(head);
+    free(stats.dinfo->mem_ptr);
+    free(dev_select);
+    return NULL;
+}
+
+GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
+{
+    return guest_get_diskstats(errp);
+}
+#endif /* CONFIG_FREEBSD */
diff --git a/qga/meson.build b/qga/meson.build
index cfa2157efb..a046ebe305 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -93,6 +93,9 @@ qga_ss = qga_ss.apply({})
 
 gen_tlb = []
 qga_libs = []
+if host_os == 'freebsd'
+  qga_libs += ['-ldevstat']
+endif
 if host_os == 'windows'
   qga_libs += ['-lws2_32', '-lwinmm', '-lpowrprof', '-lwtsapi32', '-lwininet', '-liphlpapi', '-lnetapi32',
                '-lsetupapi', '-lcfgmgr32', '-luserenv', '-lpdh' ]
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index c57bc9a02f..0d1476dcc4 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1714,7 +1714,7 @@
            '*total-ticks': 'uint64',
            '*weight-ticks': 'uint64'
            },
-  'if': 'CONFIG_LINUX' }
+  'if': { 'any': ['CONFIG_LINUX', 'CONFIG_FREEBSD'] } }
 
 ##
 # @GuestDiskStatsInfo:
@@ -1732,7 +1732,7 @@
            'major': 'uint64',
            'minor': 'uint64',
            'stats': 'GuestDiskStats' },
-  'if': 'CONFIG_LINUX' }
+  'if': { 'any': ['CONFIG_LINUX', 'CONFIG_FREEBSD'] } }
 
 ##
 # @guest-get-diskstats:
@@ -1745,7 +1745,7 @@
 ##
 { 'command': 'guest-get-diskstats',
   'returns': ['GuestDiskStatsInfo'],
-  'if': 'CONFIG_LINUX'
+  'if': { 'any': ['CONFIG_LINUX', 'CONFIG_FREEBSD'] }
 }
 
 ##
-- 
2.52.0

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

[Markus Armbruster]

Roman Bogorodskiy <bogorodskiy@gmail.com> writes:

> 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>


QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>

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

[Roman Bogorodskiy]

  Roman Bogorodskiy 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(-)

ping

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

[Kostiantyn Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 7781 bytes --]

Hi Roman,

Thanks for your patch, and sorry for the late reply


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.


> +{
> +    GuestDiskStatsInfoList **tail = *tailp;
> +    g_autofree GuestDiskStatsInfo *diskstatinfo = NULL;
> +    g_autofree GuestDiskStats *diskstat = NULL;
> +
> +    diskstatinfo = g_new0(GuestDiskStatsInfo, 1);
> +    diskstatinfo->name = g_strdup_printf("%s%d", dev->device_name,
> +                                         dev->unit_number);
> +    /*
> +     * devstat does not expose Linux-style major/minor numbers.  Report
> the
> +     * devstat device number and unit number in these mandatory QAPI
> fields.
> +     */
> +    diskstatinfo->major = dev->device_number;
> +    diskstatinfo->minor = dev->unit_number;
> +
> +    diskstat = g_new0(GuestDiskStats, 1);
> +    diskstat->has_read_ios = true;
> +    diskstat->read_ios = dev->operations[DEVSTAT_READ];
> +    diskstat->has_read_sectors = true;
> +    diskstat->read_sectors = dev->bytes[DEVSTAT_READ] / BDRV_SECTOR_SIZE;
> +    diskstat->has_read_ticks = true;
> +    diskstat->read_ticks = bintime_to_msec(&dev->duration[DEVSTAT_READ]);
> +
> +    diskstat->has_write_ios = true;
> +    diskstat->write_ios = dev->operations[DEVSTAT_WRITE];
> +    diskstat->has_write_sectors = true;
> +    diskstat->write_sectors = dev->bytes[DEVSTAT_WRITE] /
> BDRV_SECTOR_SIZE;
> +    diskstat->has_write_ticks = true;
> +    diskstat->write_ticks =
> bintime_to_msec(&dev->duration[DEVSTAT_WRITE]);
> +
> +    diskstat->has_discard_ios = true;
> +    diskstat->discard_ios = dev->operations[DEVSTAT_FREE];
> +    diskstat->has_discard_sectors = true;
> +    diskstat->discard_sectors = dev->bytes[DEVSTAT_FREE] /
> BDRV_SECTOR_SIZE;
> +    diskstat->has_discard_ticks = true;
> +    diskstat->discard_ticks =
> bintime_to_msec(&dev->duration[DEVSTAT_FREE]);
> +
> +    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?


> +
> +    diskstat->has_total_ticks = true;
> +    diskstat->total_ticks = bintime_to_msec(&dev->busy_time);
> +
> +    diskstatinfo->stats = g_steal_pointer(&diskstat);
> +    QAPI_LIST_APPEND(tail, diskstatinfo);
> +    diskstatinfo = NULL;
> +
> +    *tailp = tail;
> +}
> +
> +static GuestDiskStatsInfoList *guest_get_diskstats(Error **errp)
> +{
> +    GuestDiskStatsInfoList *head = NULL, **tail = &head;
> +    struct devinfo dinfo = { 0 };
> +    struct statinfo stats = { .dinfo = &dinfo };
> +    struct device_selection *dev_select = NULL;
> +    struct devstat_match matches[] = {
> +        {
> +            .match_fields = DEVSTAT_MATCH_TYPE,
> +            .device_type = DEVSTAT_TYPE_DIRECT,
> +            .num_match_categories = 1,
> +        },
> +    };
> +    int num_selected = 0;
> +    int num_selections = 0;
> +    long select_generation = 0;
> +    int i;
> +
> +    if (devstat_checkversion(NULL) == -1) {
> +        error_setg(errp, "%s", devstat_errbuf);
> +        return NULL;
> +    }
> +
> +    if (devstat_getdevs(NULL, &stats) == -1) {
> +        error_setg(errp, "%s", devstat_errbuf);
> +        return NULL;
> +    }
> +
> +    if (devstat_selectdevs(&dev_select, &num_selected, &num_selections,
> +                           &select_generation, stats.dinfo->generation,
> +                           stats.dinfo->devices, stats.dinfo->numdevs,
> +                           matches, ARRAY_SIZE(matches), NULL, 0,
> +                           DS_SELECT_ONLY,
> +                           stats.dinfo->numdevs, 0) == -1) {
> +        error_setg(errp, "%s", devstat_errbuf);
> +        goto error;
> +    }
> +
> +    for (i = 0; i < num_selections; i++) {
> +        if (dev_select[i].selected == 0) {
> +            continue;
> +        }
> +
> +        guest_diskstats_append(&tail,
> +
>  &stats.dinfo->devices[dev_select[i].position]);
> +    }
> +
> +    free(stats.dinfo->mem_ptr);
> +    free(dev_select);
> +    return head;
> +
> +error:
> +    qapi_free_GuestDiskStatsInfoList(head);
> +    free(stats.dinfo->mem_ptr);
> +    free(dev_select);
> +    return NULL;
> +}
> +
> +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.



> +}
> +#endif /* CONFIG_FREEBSD */
> diff --git a/qga/meson.build b/qga/meson.build
> index cfa2157efb..a046ebe305 100644
> --- a/qga/meson.build
> +++ b/qga/meson.build
> @@ -93,6 +93,9 @@ qga_ss = qga_ss.apply({})
>
>  gen_tlb = []
>  qga_libs = []
> +if host_os == 'freebsd'
> +  qga_libs += ['-ldevstat']
> +endif
>  if host_os == 'windows'
>    qga_libs += ['-lws2_32', '-lwinmm', '-lpowrprof', '-lwtsapi32',
> '-lwininet', '-liphlpapi', '-lnetapi32',
>                 '-lsetupapi', '-lcfgmgr32', '-luserenv', '-lpdh' ]
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index c57bc9a02f..0d1476dcc4 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1714,7 +1714,7 @@
>             '*total-ticks': 'uint64',
>             '*weight-ticks': 'uint64'
>             },
> -  'if': 'CONFIG_LINUX' }
> +  'if': { 'any': ['CONFIG_LINUX', 'CONFIG_FREEBSD'] } }
>
>  ##
>  # @GuestDiskStatsInfo:
> @@ -1732,7 +1732,7 @@
>             'major': 'uint64',
>             'minor': 'uint64',
>             'stats': 'GuestDiskStats' },
> -  'if': 'CONFIG_LINUX' }
> +  'if': { 'any': ['CONFIG_LINUX', 'CONFIG_FREEBSD'] } }
>
>  ##
>  # @guest-get-diskstats:
> @@ -1745,7 +1745,7 @@
>  ##
>  { 'command': 'guest-get-diskstats',
>    'returns': ['GuestDiskStatsInfo'],
> -  'if': 'CONFIG_LINUX'
> +  'if': { 'any': ['CONFIG_LINUX', 'CONFIG_FREEBSD'] }
>  }
>
>  ##
> --
> 2.52.0
>
>

Best Regards,
Kostiantyn Kostiuk.

[-- Attachment #2: Type: text/html, Size: 10639 bytes --]

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

[Roman Bogorodskiy]

  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
 

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

[Kostiantyn Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 4187 bytes --]

On Tue, Sep 1, 2026 at 8:32 PM Roman Bogorodskiy <bogorodskiy@gmail.com>
wrote:

>   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.
>

Then let's drop it


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

[-- Attachment #2: Type: text/html, Size: 5885 bytes --]