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

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

On Fri, Jul 31, 2015 at 07:36:48PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  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.

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

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