Re: [Qemu-devel] [RFC 2/3] qga: implement get-memory-info for Linux

[Marc-André Lureau]

----- Original Message -----
> On Fri, Jul 31, 2015 at 07:36:48PM +0200, address@hidden wrote:
> > From: Marc-André Lureau <address@hidden>
> > 
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  qga/commands-posix.c | 94
> >  ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  qga/main.c           | 28 ++++++++++++++++
> >  2 files changed, 120 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index eb4036e..9534c2d 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -28,6 +28,7 @@
> >  #include "qapi/qmp/qerror.h"
> >  #include "qemu/queue.h"
> >  #include "qemu/host-utils.h"
> > +#include "glib-compat.h"
> >  
> >  #ifndef CONFIG_HAS_ENVIRON
> >  #ifdef __APPLE__
> > @@ -2328,10 +2329,99 @@ GuestMemoryBlockInfo
> > *qmp_guest_get_memory_block_info(Error **errp)
> >      return info;
> >  }
> >  
> > +static long meminfo_value(gchar * const *col)
> 
> Don't we need to use uint64 here not long, since we're assigning to
> struct fields which are uint64 and on 32-bit host using long might
> result in wraparound.

Yes, thanks

> 
> > +{
> > +    int i;
> > +
> > +    g_return_val_if_fail(col && col[0], 0);
> > +
> > +    for (i = 1; col[i]; i++) {
> > +        if (strlen(col[i]) > 0) {
> > +            return g_ascii_strtoll(col[i], NULL, 10);
> > +        }
> > +    }
> > +
> > +    g_return_val_if_reached(0);
> > +}
> > +
> >  GuestMemoryInfo *qmp_guest_get_memory_info(Error **errp)
> >  {
> > -    error_setg(errp, QERR_UNSUPPORTED);
> > -    return NULL;
> > +    static guint64 last_time, last_swap_in, last_swap_out;
> > +    static guint64 last_pf_major, last_pf_minor;
> > +    GError *err = NULL;
> > +    GuestMemoryInfo *info;
> > +    gchar *contents = NULL;
> > +    gchar **lines;
> > +    int i;
> > +    guint64 time;
> > +
> > +    if (!g_file_get_contents("/proc/meminfo", &contents, NULL, &err)) {
> > +        error_setg(errp, "unable to read meminfo: %s", err->message);
> > +        g_clear_error(&err);
> > +        return NULL;
> > +    }
> > +
> > +    info = g_new0(GuestMemoryInfo, 1);
> > +
> > +    lines = g_strsplit(contents, "\n", -1);
> > +    for (i = 0; lines[i]; i++) {
> > +        gchar **col = g_strsplit(lines[i], " ", -1);
> > +        if (g_strcmp0(col[0], "MemTotal:") == 0) {
> > +            info->mem_total = meminfo_value(col);
> > +        } else if (g_strcmp0(col[0], "MemAvailable:") == 0) {
> > +            /* available since kernel 3.2 */
> > +            info->mem_free = meminfo_value(col);
> > +        } else if (g_strcmp0(col[0], "Cached:") == 0) {
> > +            info->mem_cached = meminfo_value(col);
> > +        } else if (g_strcmp0(col[0], "SwapTotal:") == 0) {
> > +            info->swap_total = meminfo_value(col);
> > +        } else if (g_strcmp0(col[0], "SwapFree:") == 0) {
> > +            info->swap_free = meminfo_value(col);
> > +        }
> > +        g_strfreev(col);
> > +    }
> > +    g_strfreev(lines);
> > +
> > +    g_free(contents);
> > +
> > +    if (!g_file_get_contents("/proc/vmstat", &contents, NULL, &err)) {
> > +        error_setg(errp, "unable to read meminfo: %s", err->message);
> > +        g_clear_error(&err);
> > +        g_free(info);
> > +        return NULL;
> > +    }
> > +
> > +    time = g_get_monotonic_time();
> > +    double elapsed = (time - last_time + 1) / (double)G_USEC_PER_SEC;
> > +
> > +#define UPDATE(Field) do {                              \
> > +    guint64 val = g_ascii_strtoll(col[1], NULL, 10);    \
> > +    info->Field = (val - last_ ##Field) / elapsed;      \
> > +    last_ ##Field = val;                                \
> > +} while (0)
> > +
> > +    lines = g_strsplit(contents, "\n", -1);
> > +    for (i = 0; lines[i]; i++) {
> > +        gchar **col = g_strsplit(lines[i], " ", -1);
> > +        if (g_strcmp0(col[0], "pswpin") == 0) {
> > +            UPDATE(swap_in);
> > +        } else if (g_strcmp0(col[0], "pswpout") == 0) {
> > +            UPDATE(swap_out);
> > +        } else if (g_strcmp0(col[0], "pgfault") == 0) {
> > +            UPDATE(pf_minor);
> > +        } else if (g_strcmp0(col[0], "pgmajfault") == 0) {
> > +            UPDATE(pf_major);
> > +        }
> > +        g_strfreev(col);
> > +    }
> > +    g_strfreev(lines);
> > +
> > +#undef UPDATE
> > +
> > +    last_time = time;
> > +    g_free(contents);
> > +
> > +    return info;
> >  }
> >  
> >  #else /* defined(__linux__) */
> > diff --git a/qga/main.c b/qga/main.c
> > index aef007b..4790e26 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -42,6 +42,7 @@
> >  #define CONFIG_FSFREEZE
> >  #endif
> >  #endif
> > +#include "qga-qmp-commands.h"
> >  
> >  #ifndef _WIN32
> >  #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
> > @@ -901,6 +902,31 @@ int64_t ga_get_fd_handle(GAState *s, Error **errp)
> >      return handle;
> >  }
> >  
> > +static void initialize_memory_stats(void)
> > +{
> > +    GuestMemoryInfo *info = qmp_guest_get_memory_info(NULL);
> > +
> > +    if (!info) {
> > +        return;
> > +    }
> > +
> > +    /* just for checking at start if everything looks ok */
> > +    g_debug("mem-total: %" G_GUINT64_FORMAT " kB\n"
> > +            "mem-free: %" G_GUINT64_FORMAT " kB\n"
> > +            "mem-cached: %" G_GUINT64_FORMAT " kB\n"
> > +            "swap-total: %" G_GUINT64_FORMAT " kB\n"
> > +            "swap-free: %" G_GUINT64_FORMAT " kB\n"
> > +            "swap-in: %" G_GUINT64_FORMAT " kB\n"
> > +            "swap-out: %" G_GUINT64_FORMAT " kB\n"
> > +            "pf-major: %" G_GUINT64_FORMAT " kB\n"
> > +            "pf-minor: %" G_GUINT64_FORMAT " kB\n",
> > +            info->mem_total, info->mem_free, info->mem_cached,
> > +            info->swap_total, info->swap_free, info->swap_in,
> > info->swap_out,
> > +            info->pf_major, info->pf_minor);
> > +
> > +    g_free(info);
> > +}
> 
> Wouldn't we be better off just adding a proper unit test for
> the code. We could copy a few same /proc/meminfo commands from
> various different Linux architectures and versions and then
> test parsing of them.

Sure, but a debug dump is still useful, doesn't hurt much. It doesn' look like 
there are unit tests for qga. It'd something worth to look at.

> > +
> >  static void ga_print_cmd(QmpCommand *cmd, void *opaque)
> >  {
> >      printf("%s\n", qmp_command_name(cmd));
> > @@ -1256,6 +1282,8 @@ static int run_agent(GAState *s)
> >      }
> >  #endif
> >  
> > +    initialize_memory_stats();
> > +
> >      s->main_loop = g_main_loop_new(NULL, false);
> >      if (!channel_init(ga_state, method, device_path)) {
> >          g_critical("failed to initialize guest agent channel");
> 
> 
> Regards,
> Daniel
> --
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
>