[Qemu-devel] [PATCH RFC v2 00/12] VMState testing

[Qemu-devel] [PATCH RFC v2 00/12] VMState testing

[Sanidhya Kashyap]

Hi,

The following patch introduce a mechanism to test the correctness of the
vmstate's information. This is achieved by saving the device states'
information to a memory buffer and then clearing the states, followed by
loading the data from the saved memory buffer.

v1 --> v2:

* Added a list containing all the devices that have been qdevified and gets
  registered with the SaveStateEntry.
* Have provided a way to use either qemu_system_reset functionality or my
  own version of qdev entries untill all the devices have been qdevified.
  This gives us the privilege to test any device we want.
* Rename some of the variables. I am very bad at naming convention, thanks to
  community, specially Eric, I try to improve it with every version.
* On Eric's advice, I have separated all of the qmp and hmp interface patches.
* Changed the DPRINTF statements as required.

Dr. David Alan Gilbert (1):
  QEMUSizedBuffer/QEMUFile

Sanidhya Kashyap (11):
  reset handler for qdevified devices
  VMState test: query command to extract the qdevified device names
  VMState test: hmp interface for showing qdevified devices
  VMstate test: basic VMState testing mechanism
  VMState test: hmp interface for vmstate testing
  VMState test: qmp interface for querying the vmstate testing process
  VMState test: hmp interface for querying the vmstate testing process
  VMState test: update period of vmstate testing process
  VMState test: hmp interface for period update
  VMState test: cancel mechanism for an already running vmstate testing
    process
  VMState test: hmp interface for cancel mechanism

 hmp-commands.hx               |  48 +++++
 hmp.c                         |  73 ++++++++
 hmp.h                         |   5 +
 include/migration/qemu-file.h |  27 +++
 monitor.c                     |  14 ++
 qapi-schema.json              | 103 +++++++++++
 qemu-file.c                   | 411 ++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx               | 129 +++++++++++++
 savevm.c                      | 357 ++++++++++++++++++++++++++++++++++++
 9 files changed, 1167 insertions(+)

-- 
1.9.3

[Qemu-devel] [PATCH RFC v2 01/12] QEMUSizedBuffer/QEMUFile

[Sanidhya Kashyap]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Stefan Berger's to create a QEMUFile that goes to a memory buffer;
from:

http://lists.gnu.org/archive/html/qemu-devel/2013-03/msg05036.html

Using the QEMUFile interface, this patch adds support functions for
operating
on in-memory sized buffers that can be written to or read from.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Joel Schopp <jschopp@linux.vnet.ibm.com>
---
 include/migration/qemu-file.h |  27 +++
 qemu-file.c                   | 411 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 438 insertions(+)

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index c90f529..14e1f4d 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -25,6 +25,8 @@
 #define QEMU_FILE_H 1
 #include "exec/cpu-common.h"
 
+#include <stdint.h>
+
 /* This function writes a chunk of data to a file at the given position.
  * The pos argument can be ignored if the file is only being used for
  * streaming.  The handler should try to write all of the data it can.
@@ -94,11 +96,31 @@ typedef struct QEMUFileOps {
     QEMURamSaveFunc *save_page;
 } QEMUFileOps;
 
+struct QEMUSizedBuffer {
+    struct iovec *iov;
+    size_t n_iov;
+    size_t size; /* total allocated size in all iov's */
+    size_t used; /* number of used bytes */
+};
+
+typedef struct QEMUSizedBuffer QEMUSizedBuffer;
+QEMUSizedBuffer *qsb_create(const uint8_t *buffer, size_t len);
+QEMUSizedBuffer *qsb_clone(const QEMUSizedBuffer *);
+void qsb_free(QEMUSizedBuffer *);
+size_t qsb_set_length(QEMUSizedBuffer *qsb, size_t length);
+size_t qsb_get_length(const QEMUSizedBuffer *qsb);
+ssize_t qsb_get_buffer(const QEMUSizedBuffer *, off_t start, size_t count,
+                       uint8_t **buf);
+ssize_t qsb_write_at(QEMUSizedBuffer *qsb, const uint8_t *buf,
+                     off_t pos, size_t count);
+
+
 QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
 QEMUFile *qemu_fopen(const char *filename, const char *mode);
 QEMUFile *qemu_fdopen(int fd, const char *mode);
 QEMUFile *qemu_fopen_socket(int fd, const char *mode);
 QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
+QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input);
 int qemu_get_fd(QEMUFile *f);
 int qemu_fclose(QEMUFile *f);
 int64_t qemu_ftell(QEMUFile *f);
@@ -111,6 +133,11 @@ void qemu_put_byte(QEMUFile *f, int v);
 void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size);
 bool qemu_file_mode_is_not_valid(const char *mode);
 
+/*
+ * For use on files opened with qemu_bufopen
+ */
+const QEMUSizedBuffer *qemu_buf_get(QEMUFile *f);
+
 static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
 {
     qemu_put_byte(f, (int)v);
diff --git a/qemu-file.c b/qemu-file.c
index a8e3912..9fd1675 100644
--- a/qemu-file.c
+++ b/qemu-file.c
@@ -878,3 +878,414 @@ uint64_t qemu_get_be64(QEMUFile *f)
     v |= qemu_get_be32(f);
     return v;
 }
+
+
+#define QSB_CHUNK_SIZE      (1 << 10)
+#define QSB_MAX_CHUNK_SIZE  (10 * QSB_CHUNK_SIZE)
+
+/**
+ * Create a QEMUSizedBuffer
+ * This type of buffer uses scatter-gather lists internally and
+ * can grow to any size. Any data array in the scatter-gather list
+ * can hold different amount of bytes.
+ *
+ * @buffer: Optional buffer to copy into the QSB
+ * @len: size of initial buffer; if @buffer is given, buffer must
+ *       hold at least len bytes
+ *
+ * Returns a pointer to a QEMUSizedBuffer
+ */
+QEMUSizedBuffer *qsb_create(const uint8_t *buffer, size_t len)
+{
+    QEMUSizedBuffer *qsb;
+    size_t alloc_len, num_chunks, i, to_copy;
+    size_t chunk_size = (len > QSB_MAX_CHUNK_SIZE)
+                        ? QSB_MAX_CHUNK_SIZE
+                        : QSB_CHUNK_SIZE;
+
+    if (len == 0) {
+        /* we want to allocate at least one chunk */
+        len = QSB_CHUNK_SIZE;
+    }
+
+    num_chunks = DIV_ROUND_UP(len, chunk_size);
+    alloc_len = num_chunks * chunk_size;
+
+    qsb = g_new0(QEMUSizedBuffer, 1);
+    qsb->iov = g_new0(struct iovec, num_chunks);
+    qsb->n_iov = num_chunks;
+
+    for (i = 0; i < num_chunks; i++) {
+        qsb->iov[i].iov_base = g_malloc0(chunk_size);
+        qsb->iov[i].iov_len = chunk_size;
+        if (buffer) {
+            to_copy = (len - qsb->used) > chunk_size
+                      ? chunk_size : (len - qsb->used);
+            memcpy(qsb->iov[i].iov_base, &buffer[qsb->used], to_copy);
+            qsb->used += to_copy;
+        }
+    }
+
+    qsb->size = alloc_len;
+
+    return qsb;
+}
+
+/**
+ * Free the QEMUSizedBuffer
+ *
+ * @qsb: The QEMUSizedBuffer to free
+ */
+void qsb_free(QEMUSizedBuffer *qsb)
+{
+    size_t i;
+
+    if (!qsb) {
+        return;
+    }
+
+    for (i = 0; i < qsb->n_iov; i++) {
+        g_free(qsb->iov[i].iov_base);
+    }
+    g_free(qsb->iov);
+    g_free(qsb);
+}
+
+/**
+ * Get the number of of used bytes in the QEMUSizedBuffer
+ *
+ * @qsb: A QEMUSizedBuffer
+ *
+ * Returns the number of bytes currently used in this buffer
+ */
+size_t qsb_get_length(const QEMUSizedBuffer *qsb)
+{
+    return qsb->used;
+}
+
+/**
+ * Set the length of the buffer; The primary usage of this
+ * function is to truncate the number of used bytes in the buffer.
+ * The size will not be extended beyond the current  number of
+ * allocated bytes in the QEMUSizedBuffer.
+ *
+ * @qsb: A QEMUSizedBuffer
+ * @new_len : The new length of bytes in the buffer
+ *
+ * Returns the number of bytes the buffer was trucated or extended
+ * to.
+ */
+size_t qsb_set_length(QEMUSizedBuffer *qsb, size_t new_len)
+{
+    if (new_len <= qsb->size) {
+        qsb->used = new_len;
+    } else {
+        qsb->used = qsb->size;
+    }
+    return qsb->used;
+}
+
+/**
+ * Get the iovec that holds the data for a given position @pos.
+ *
+ * @qsb: A QEMUSizedBuffer
+ * @pos: The index of a byte in the buffer
+ * @d_off: Pointer to an offset that this function will indicate
+ *         at what position within the returned iovec the byte
+ *         is to be found
+ *
+ * Returns the index of the iovec that holds the byte at the given
+ * index @pos in the byte stream; a negative number if the iovec
+ * for the given position @pos does not exist.
+ */
+static ssize_t qsb_get_iovec(const QEMUSizedBuffer *qsb,
+                             off_t pos, off_t *d_off)
+{
+    ssize_t i;
+    off_t curr = 0;
+
+    if (pos > qsb->used) {
+        return -1;
+    }
+
+    for (i = 0; i < qsb->n_iov; i++) {
+        if (curr + qsb->iov[i].iov_len > pos) {
+            *d_off = pos - curr;
+            return i;
+        }
+        curr += qsb->iov[i].iov_len;
+    }
+    return -1;
+}
+
+/*
+ * Convert the QEMUSizedBuffer into a flat buffer.
+ *
+ * Note: If at all possible, try to avoid this function since it
+ *       may unnecessarily copy memory around.
+ *
+ * @qsb: pointer to QEMUSizedBuffer
+ * @start : offset to start at
+ * @count: number of bytes to copy
+ * @buf: a pointer to an optional buffer to write into; the pointer may
+ *       point to NULL in which case the buffer will be allocated;
+ *       if buffer is provided, it must be large enough to hold @count bytes
+ *
+ * Returns the number of bytes  copied into the output buffer
+ */
+ssize_t qsb_get_buffer(const QEMUSizedBuffer *qsb, off_t start,
+                       size_t count, uint8_t **buf)
+{
+    uint8_t *buffer;
+    const struct iovec *iov;
+    size_t to_copy, all_copy;
+    ssize_t index;
+    off_t s_off;
+    off_t d_off = 0;
+    char *s;
+
+    if (start > qsb->used) {
+        return 0;
+    }
+
+    all_copy = qsb->used - start;
+    if (all_copy > count) {
+        all_copy = count;
+    } else {
+        count = all_copy;
+    }
+
+    if (*buf == NULL) {
+        *buf = g_malloc(all_copy);
+    }
+    buffer = *buf;
+
+    index = qsb_get_iovec(qsb, start, &s_off);
+    if (index < 0) {
+        return 0;
+    }
+
+    while (all_copy > 0) {
+        iov = &qsb->iov[index];
+
+        s = iov->iov_base;
+
+        to_copy = iov->iov_len - s_off;
+        if (to_copy > all_copy) {
+            to_copy = all_copy;
+        }
+        memcpy(&buffer[d_off], &s[s_off], to_copy);
+
+        d_off += to_copy;
+        all_copy -= to_copy;
+
+        s_off = 0;
+        index++;
+    }
+
+    return count;
+}
+
+/**
+ * Grow the QEMUSizedBuffer to the given size and allocated
+ * memory for it.
+ *
+ * @qsb: A QEMUSizedBuffer
+ * @new_size: The new size of the buffer
+ *
+ * Returns an error code in case of memory allocation failure
+ * or the new size of the buffer otherwise. The returned size
+ * may be greater or equal to @new_size.
+ */
+static ssize_t qsb_grow(QEMUSizedBuffer *qsb, size_t new_size)
+{
+    size_t needed_chunks, i;
+    size_t chunk_size = QSB_CHUNK_SIZE;
+
+    if (qsb->size < new_size) {
+        needed_chunks = DIV_ROUND_UP(new_size - qsb->size,
+                                     chunk_size);
+
+        qsb->iov = g_realloc_n(qsb->iov, qsb->n_iov + needed_chunks,
+                               sizeof(struct iovec));
+        if (qsb->iov == NULL) {
+            return -ENOMEM;
+        }
+
+        for (i = qsb->n_iov; i < qsb->n_iov + needed_chunks; i++) {
+            qsb->iov[i].iov_base = g_malloc0(chunk_size);
+            qsb->iov[i].iov_len = chunk_size;
+        }
+
+        qsb->n_iov += needed_chunks;
+        qsb->size += (needed_chunks * chunk_size);
+    }
+
+    return qsb->size;
+}
+
+/**
+ * Write into the QEMUSizedBuffer at a given position and a given
+ * number of bytes. This function will automatically grow the
+ * QEMUSizedBuffer.
+ *
+ * @qsb: A QEMUSizedBuffer
+ * @source: A byte array to copy data from
+ * @pos: The position withing the @qsb to write data to
+ * @size: The number of bytes to copy into the @qsb
+ *
+ * Returns an error code in case of memory allocation failure,
+ * @size otherwise.
+ */
+ssize_t qsb_write_at(QEMUSizedBuffer *qsb, const uint8_t *source,
+                     off_t pos, size_t count)
+{
+    ssize_t rc = qsb_grow(qsb, pos + count);
+    size_t to_copy;
+    size_t all_copy = count;
+    const struct iovec *iov;
+    ssize_t index;
+    char *dest;
+    off_t d_off, s_off = 0;
+
+    if (rc < 0) {
+        return rc;
+    }
+
+    if (pos + count > qsb->used) {
+        qsb->used = pos + count;
+    }
+
+    index = qsb_get_iovec(qsb, pos, &d_off);
+    if (index < 0) {
+        return 0;
+    }
+
+    while (all_copy > 0) {
+        iov = &qsb->iov[index];
+
+        dest = iov->iov_base;
+
+        to_copy = iov->iov_len - d_off;
+        if (to_copy > all_copy) {
+            to_copy = all_copy;
+        }
+
+        memcpy(&dest[d_off], &source[s_off], to_copy);
+
+        s_off += to_copy;
+        all_copy -= to_copy;
+
+        d_off = 0;
+        index++;
+    }
+
+    return count;
+}
+
+/**
+ * Create an exact copy of the given QEMUSizedBuffer.
+ *
+ * @qsb : A QEMUSizedBuffer
+ *
+ * Returns a clone of @qsb
+ */
+QEMUSizedBuffer *qsb_clone(const QEMUSizedBuffer *qsb)
+{
+    QEMUSizedBuffer *out = qsb_create(NULL, qsb_get_length(qsb));
+    size_t i;
+    off_t pos = 0;
+
+    for (i = 0; i < qsb->n_iov; i++) {
+        pos += qsb_write_at(out, qsb->iov[i].iov_base,
+                            pos, qsb->iov[i].iov_len);
+    }
+
+    return out;
+}
+
+typedef struct QEMUBuffer {
+    QEMUSizedBuffer *qsb;
+    QEMUFile *file;
+} QEMUBuffer;
+
+static int buf_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
+{
+    QEMUBuffer *s = opaque;
+    ssize_t len = qsb_get_length(s->qsb) - pos;
+
+    if (len <= 0) {
+        return 0;
+    }
+
+    if (len > size) {
+        len = size;
+    }
+    return qsb_get_buffer(s->qsb, pos, len, &buf);
+}
+
+static int buf_put_buffer(void *opaque, const uint8_t *buf,
+                          int64_t pos, int size)
+{
+    QEMUBuffer *s = opaque;
+
+    return qsb_write_at(s->qsb, buf, pos, size);
+}
+
+static int buf_close(void *opaque)
+{
+    QEMUBuffer *s = opaque;
+
+    qsb_free(s->qsb);
+
+    g_free(s);
+
+    return 0;
+}
+
+const QEMUSizedBuffer *qemu_buf_get(QEMUFile *f)
+{
+    QEMUBuffer *p;
+
+    qemu_fflush(f);
+
+    p = (QEMUBuffer *)f->opaque;
+
+    return p->qsb;
+}
+
+static const QEMUFileOps buf_read_ops = {
+    .get_buffer = buf_get_buffer,
+    .close =      buf_close
+};
+
+static const QEMUFileOps buf_write_ops = {
+    .put_buffer = buf_put_buffer,
+    .close =      buf_close
+};
+
+QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input)
+{
+    QEMUBuffer *s;
+
+    if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) {
+        fprintf(stderr, "qemu_bufopen: Argument validity check failed\n");
+        return NULL;
+    }
+
+    s = g_malloc0(sizeof(QEMUBuffer));
+    if (mode[0] == 'r') {
+        s->qsb = input;
+    }
+
+    if (s->qsb == NULL) {
+        s->qsb = qsb_create(NULL, 0);
+    }
+
+    if (mode[0] == 'r') {
+        s->file = qemu_fopen_ops(s, &buf_read_ops);
+    } else {
+        s->file = qemu_fopen_ops(s, &buf_write_ops);
+    }
+    return s->file;
+}
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC v2 01/12] QEMUSizedBuffer/QEMUFile

[Eric Blake]

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

On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Stefan Berger's to create a QEMUFile that goes to a memory buffer;

Missing something.  Maybe you meant:

This is based on Stefan Berger's patch to create ...

> from:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2013-03/msg05036.html
> 
> Using the QEMUFile interface, this patch adds support functions for
> operating
> on in-memory sized buffers that can be written to or read from.

Odd line breaking.

> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Joel Schopp <jschopp@linux.vnet.ibm.com>

Still looks weird to have David as author but not listed in S-o-B.

> ---
>  include/migration/qemu-file.h |  27 +++
>  qemu-file.c                   | 411 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 438 insertions(+)
> 

> +/**
> + * Set the length of the buffer; The primary usage of this

s/The/the/

> + * function is to truncate the number of used bytes in the buffer.
> + * The size will not be extended beyond the current  number of

no need for double space

> + * allocated bytes in the QEMUSizedBuffer.
> + *
> + * @qsb: A QEMUSizedBuffer
> + * @new_len : The new length of bytes in the buffer
> + *
> + * Returns the number of bytes the buffer was trucated or extended

s/trucated/truncated/

> +
> +QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input)
> +{
> +    QEMUBuffer *s;
> +
> +    if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) {
> +        fprintf(stderr, "qemu_bufopen: Argument validity check failed\n");

Should this function be directly printing to stderr, or should it be
converted to use Error **errp semantics?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH RFC v2 01/12] QEMUSizedBuffer/QEMUFile

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Stefan Berger's to create a QEMUFile that goes to a memory buffer;
> 
> Missing something.  Maybe you meant:
> 
> This is based on Stefan Berger's patch to create ...

I'll update that in my version.

> > from:
> > 
> > http://lists.gnu.org/archive/html/qemu-devel/2013-03/msg05036.html
> > 
> > Using the QEMUFile interface, this patch adds support functions for
> > operating
> > on in-memory sized buffers that can be written to or read from.
> 
> Odd line breaking.
> 
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > Signed-off-by: Joel Schopp <jschopp@linux.vnet.ibm.com>
> 
> Still looks weird to have David as author but not listed in S-o-B.

I'm thinking of trying to put this code in via a stand-alone patch
(with a testcase use); since we've got 3+ users of this code in unmerged
things and at least 3 implementations of something similar, it would
be best to get it in before someone writes a 4th.

> > ---
> >  include/migration/qemu-file.h |  27 +++
> >  qemu-file.c                   | 411 ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 438 insertions(+)
> > 
> 
> > +/**
> > + * Set the length of the buffer; The primary usage of this
> 
> s/The/the/

> > + * function is to truncate the number of used bytes in the buffer.
> > + * The size will not be extended beyond the current  number of
> 
> no need for double space
> 
> > + * allocated bytes in the QEMUSizedBuffer.
> > + *
> > + * @qsb: A QEMUSizedBuffer
> > + * @new_len : The new length of bytes in the buffer
> > + *
> > + * Returns the number of bytes the buffer was trucated or extended
> 
> s/trucated/truncated/

I've picked up these typos in my world.  Thanks.

> > +
> > +QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input)
> > +{
> > +    QEMUBuffer *s;
> > +
> > +    if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) {
> > +        fprintf(stderr, "qemu_bufopen: Argument validity check failed\n");
> 
> Should this function be directly printing to stderr, or should it be
> converted to use Error **errp semantics?

It's consistent with the open functions for other file types
 ( qemu_fdopen, qemu_popen_cmd, qemu_fopen ).

Dave

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH RFC v2 02/12] reset handler for qdevified devices

[Sanidhya Kashyap]

I have added a structure containing the list of qdevified devices which have
been added to the SaveVMHandlers. Since, I was unable to find any particular
struct containing the information about all the qdevified devices. So, I have
created my own version, which is very very specific.

I shall be grateful if anyone can give some pointers on this.

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 savevm.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/savevm.c b/savevm.c
index e19ae0a..0255fa0 100644
--- a/savevm.c
+++ b/savevm.c
@@ -503,12 +503,29 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
     }
 }
 
+/*
+ * Reset entry for qdevified devices.
+ * Contains all those devices which have been qdevified and are
+ * part of the SaveVMHandlers. This one allows resetting of
+ * single device at any time.
+ */
+
+typedef struct VMStatesQdevResetEntry {
+    QTAILQ_ENTRY(VMStatesQdevResetEntry) entry;
+    DeviceState *dev;
+    char device_name[256];
+} VMStatesQdevResetEntry;
+
+static QTAILQ_HEAD(vmstate_reset_handlers, VMStatesQdevResetEntry)
+      vmstate_reset_handlers = QTAILQ_HEAD_INITIALIZER(vmstate_reset_handlers);
+
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque, int alias_id,
                                    int required_for_version)
 {
     SaveStateEntry *se;
+    VMStatesQdevResetEntry *qre;
 
     /* If this triggers, alias support can be dropped for the vmsd. */
     assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
@@ -521,6 +538,12 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
     se->alias_id = alias_id;
 
     if (dev) {
+
+        qre = g_malloc0(sizeof(VMStatesQdevResetEntry));
+        qre->dev = dev;
+        strcpy(qre->device_name, vmsd->name);
+        QTAILQ_INSERT_TAIL(&vmstate_reset_handlers, qre, entry);
+
         char *id = qdev_get_dev_path(dev);
         if (id) {
             pstrcpy(se->idstr, sizeof(se->idstr), id);
@@ -551,6 +574,7 @@ void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
                         void *opaque)
 {
     SaveStateEntry *se, *new_se;
+    VMStatesQdevResetEntry *qre, *new_qre;
 
     QTAILQ_FOREACH_SAFE(se, &savevm_handlers, entry, new_se) {
         if (se->vmsd == vmsd && se->opaque == opaque) {
@@ -561,6 +585,12 @@ void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
             g_free(se);
         }
     }
+
+    QTAILQ_FOREACH_SAFE(qre, &vmstate_reset_handlers, entry, new_qre) {
+        if (dev && qre->dev && !strcmp(vmsd->name, qre->device_name)) {
+            QTAILQ_REMOVE(&vmstate_reset_handlers, qre, entry);
+        }
+    }
 }
 
 static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC v2 02/12] reset handler for qdevified devices

[Juan Quintela]

Sanidhya Kashyap <sanidhya.iiith@gmail.com> wrote:
> I have added a structure containing the list of qdevified devices which have
> been added to the SaveVMHandlers. Since, I was unable to find any particular
> struct containing the information about all the qdevified devices. So, I have
> created my own version, which is very very specific.
>
> I shall be grateful if anyone can give some pointers on this.
>
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---
>  savevm.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/savevm.c b/savevm.c
> index e19ae0a..0255fa0 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -503,12 +503,29 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
>      }
>  }
>  
> +/*
> + * Reset entry for qdevified devices.
> + * Contains all those devices which have been qdevified and are
> + * part of the SaveVMHandlers. This one allows resetting of
> + * single device at any time.
> + */
> +
> +typedef struct VMStatesQdevResetEntry {
> +    QTAILQ_ENTRY(VMStatesQdevResetEntry) entry;
> +    DeviceState *dev;
> +    char device_name[256];
> +} VMStatesQdevResetEntry;
> +
> +static QTAILQ_HEAD(vmstate_reset_handlers, VMStatesQdevResetEntry)
> +      vmstate_reset_handlers = QTAILQ_HEAD_INITIALIZER(vmstate_reset_handlers);
> +
>  int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *opaque, int alias_id,
>                                     int required_for_version)
>  {
>      SaveStateEntry *se;
> +    VMStatesQdevResetEntry *qre;
>  
>      /* If this triggers, alias support can be dropped for the vmsd. */
>      assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
> @@ -521,6 +538,12 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>      se->alias_id = alias_id;
>  
>      if (dev) {
> +
> +        qre = g_malloc0(sizeof(VMStatesQdevResetEntry));
> +        qre->dev = dev;
> +        strcpy(qre->device_name, vmsd->name);
> +        QTAILQ_INSERT_TAIL(&vmstate_reset_handlers, qre, entry);
> +

As stated on irc, you could add a "dev" field to SaveStateEntry and call
it a day.

Thanks, Juan.


>          char *id = qdev_get_dev_path(dev);
>          if (id) {
>              pstrcpy(se->idstr, sizeof(se->idstr), id);
> @@ -551,6 +574,7 @@ void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
>                          void *opaque)
>  {
>      SaveStateEntry *se, *new_se;
> +    VMStatesQdevResetEntry *qre, *new_qre;
>  
>      QTAILQ_FOREACH_SAFE(se, &savevm_handlers, entry, new_se) {
>          if (se->vmsd == vmsd && se->opaque == opaque) {
> @@ -561,6 +585,12 @@ void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
>              g_free(se);
>          }
>      }
> +
> +    QTAILQ_FOREACH_SAFE(qre, &vmstate_reset_handlers, entry, new_qre) {
> +        if (dev && qre->dev && !strcmp(vmsd->name, qre->device_name)) {
> +            QTAILQ_REMOVE(&vmstate_reset_handlers, qre, entry);
> +        }
> +    }
>  }
>  
>  static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)

[Qemu-devel] [PATCH RFC v2 03/12] VMState test: query command to extract the qdevified device names

[Sanidhya Kashyap]

I have provided a qmp interface for getting the list of qdevified devices
that have been registered with SaveVMHandlers.

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 qapi-schema.json | 22 ++++++++++++++++++++++
 qmp-commands.hx  | 25 +++++++++++++++++++++++++
 savevm.c         | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index b11aad2..996e6b5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3480,3 +3480,25 @@
 # Since: 2.1
 ##
 { 'command': 'rtc-reset-reinjection' }
+
+##
+# @VMstatesQdevDevices
+#
+# list of qdevified devices that are registered with SaveStateEntry
+#
+# @device: list of qdevified device names
+#
+# Since 2.2
+##
+{ 'type': 'VMStatesQdevDevices',
+  'data': { 'device': ['str'] } }
+
+##
+# @query-qdev-devices
+#
+# returns the VMStatesQdevDevices that have the associated value
+#
+# Since 2.2
+##
+{ 'command': 'query-qdev-devices',
+  'returns': 'VMStatesQdevDevices' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4be4765..2e20032 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3755,3 +3755,28 @@ Example:
 <- { "return": {} }
 
 EQMP
+
+    {
+        .name       = "query-qdev-devices",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_qdev_devices,
+    },
+
+SQMP
+query-qdev-devices
+------------------
+
+Shows registered Qdevified devices
+
+
+Example (1):
+
+-> { "execute": "query-qdev-devices" }
+<- { "return": [
+       {
+         "devices": [ "kvm-tpr-opt", "piix4_pm" ]
+       }
+     ]
+   }
+
+EQMP
diff --git a/savevm.c b/savevm.c
index 0255fa0..7c1600a 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1167,6 +1167,40 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }
 }
 
+static strList *create_qdev_list(const char *name, strList *list)
+{
+    strList *temp_list;
+    int len;
+
+    if (!list) {
+        list = g_malloc0(sizeof(strList));
+        len = strlen(name);
+        list->value = g_malloc0(sizeof(char)*(len+1));
+        strcpy(list->value, name);
+        list->next = NULL;
+        return list;
+    }
+    temp_list = g_malloc0(sizeof(strList));
+    len = strlen(name);
+    temp_list->value = g_malloc0(sizeof(char)*(len+1));
+    strcpy(temp_list->value, name);
+    temp_list->next = list;
+    list = temp_list;
+    return list;
+}
+
+VMStatesQdevDevices *qmp_query_qdev_devices(Error **errp)
+{
+    VMStatesQdevResetEntry *qre;
+    VMStatesQdevDevices *qdev_devices = g_malloc0(sizeof(VMStatesQdevDevices));
+
+    QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) {
+        qdev_devices->device = create_qdev_list(qre->device_name,
+                                                 qdev_devices->device);
+    }
+    return qdev_devices;
+}
+
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC v2 03/12] VMState test: query command to extract the qdevified device names

[Eric Blake]

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

On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
> I have provided a qmp interface for getting the list of qdevified devices
> that have been registered with SaveVMHandlers.
> 
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---
>  qapi-schema.json | 22 ++++++++++++++++++++++
>  qmp-commands.hx  | 25 +++++++++++++++++++++++++
>  savevm.c         | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+)
> 

> +# @device: list of qdevified device names
> +#
> +# Since 2.2
> +##
> +{ 'type': 'VMStatesQdevDevices',
> +  'data': { 'device': ['str'] } }

Here, you name it 'device' [1]

> +
> +##
> +# @query-qdev-devices
> +#
> +# returns the VMStatesQdevDevices that have the associated value
> +#
> +# Since 2.2
> +##
> +{ 'command': 'query-qdev-devices',
> +  'returns': 'VMStatesQdevDevices' }

and state that it returns a single struct [2]

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4be4765..2e20032 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3755,3 +3755,28 @@ Example:
>  <- { "return": {} }
>  
>  EQMP
> +
> +    {
> +        .name       = "query-qdev-devices",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_qdev_devices,
> +    },
> +
> +SQMP
> +query-qdev-devices
> +------------------
> +
> +Shows registered Qdevified devices
> +
> +
> +Example (1):
> +
> +-> { "execute": "query-qdev-devices" }
> +<- { "return": [

But here, your example shows it returning an array of structs [2]

> +       {
> +         "devices": [ "kvm-tpr-opt", "piix4_pm" ]

where each struct contains a member named 'devices' [1] that is also an
array.

I actually think the most extensible thing would be to return something
more like this QMP wire contents:

{ "return": [
  { "device": "kvm-tpr-opt" },
  { "device": "piix4_pm" }
] }

which would match this .json content:

{ 'type': 'VMStatesQdevDevices',
  'data': { 'device': 'str' } }
{ 'command': 'query-qdev-devices',
  'returns': [ 'VMStatesQdevDevices' ] }

and also be the most extensible for future tweaks, such as adding an
optional boolean flag to the json to tell us more about certain devices:

{ 'type': 'VMStatesQdevDevices',
  'data': { 'device': 'str', '*foo': 'bool' } }

and lead to this QMP wire transaction:

{ "return": [
  { "device": "kvm-tpr-opt" },
  { "device": "piix4_pm", 'foo': true }
] }


At any rate, you MUST make your example match your documentation.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH RFC v2 03/12] VMState test: query command to extract the qdevified device names

[Juan Quintela]

Sanidhya Kashyap <sanidhya.iiith@gmail.com> wrote:
> I have provided a qmp interface for getting the list of qdevified devices
> that have been registered with SaveVMHandlers.
>
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---
>  qapi-schema.json | 22 ++++++++++++++++++++++
>  qmp-commands.hx  | 25 +++++++++++++++++++++++++
>  savevm.c         | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b11aad2..996e6b5 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3480,3 +3480,25 @@
>  # Since: 2.1
>  ##
>  { 'command': 'rtc-reset-reinjection' }
> +
> +##
> +# @VMstatesQdevDevices
> +#
> +# list of qdevified devices that are registered with SaveStateEntry
> +#
> +# @device: list of qdevified device names


Should we use qdev on the name?  Or just list of devices?  My
understanding is that all devices are on this list, no?

> +#
> +# Since 2.2
> +##
> +{ 'type': 'VMStatesQdevDevices',
> +  'data': { 'device': ['str'] } }
> +
> +##
> +# @query-qdev-devices
> +#
> +# returns the VMStatesQdevDevices that have the associated value
> +#
> +# Since 2.2
> +##
> +{ 'command': 'query-qdev-devices',
> +  'returns': 'VMStatesQdevDevices' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4be4765..2e20032 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3755,3 +3755,28 @@ Example:
>  <- { "return": {} }
>  
>  EQMP
> +
> +    {
> +        .name       = "query-qdev-devices",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_qdev_devices,
> +    },
> +
> +SQMP
> +query-qdev-devices
> +------------------
> +
> +Shows registered Qdevified devices
> +
> +
> +Example (1):
> +
> +-> { "execute": "query-qdev-devices" }
> +<- { "return": [
> +       {
> +         "devices": [ "kvm-tpr-opt", "piix4_pm" ]

Once here, can we change this to also include the device version?

i.e. something like:

 "devices": [ [ "device": [ "name": "kvm-tpr-opt", "version", 15]]], ...]

Or somesuch?

> +       }
> +     ]
> +   }
> +
> +EQMP
> diff --git a/savevm.c b/savevm.c
> index 0255fa0..7c1600a 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1167,6 +1167,40 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +static strList *create_qdev_list(const char *name, strList *list)
> +{
> +    strList *temp_list;
> +    int len;
> +
> +    if (!list) {
> +        list = g_malloc0(sizeof(strList));
> +        len = strlen(name);
> +        list->value = g_malloc0(sizeof(char)*(len+1));
> +        strcpy(list->value, name);
> +        list->next = NULL;
> +        return list;
> +    }
> +    temp_list = g_malloc0(sizeof(strList));
> +    len = strlen(name);
> +    temp_list->value = g_malloc0(sizeof(char)*(len+1));
> +    strcpy(temp_list->value, name);
> +    temp_list->next = list;
> +    list = temp_list;
> +    return list;
> +}
> +
> +VMStatesQdevDevices *qmp_query_qdev_devices(Error **errp)
> +{
> +    VMStatesQdevResetEntry *qre;
> +    VMStatesQdevDevices *qdev_devices = g_malloc0(sizeof(VMStatesQdevDevices));
> +
> +    QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) {
> +        qdev_devices->device = create_qdev_list(qre->device_name,
> +                                                 qdev_devices->device);
> +    }
> +    return qdev_devices;
> +}
> +
>  void qmp_xen_save_devices_state(const char *filename, Error **errp)
>  {
>      QEMUFile *f;

Re: [Qemu-devel] [PATCH RFC v2 03/12] VMState test: query command to extract the qdevified device names

[Eric Blake]

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

On 07/29/2014 06:45 AM, Juan Quintela wrote:
> Sanidhya Kashyap <sanidhya.iiith@gmail.com> wrote:
>> I have provided a qmp interface for getting the list of qdevified devices
>> that have been registered with SaveVMHandlers.
>>
>> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
>> ---
>>  qapi-schema.json | 22 ++++++++++++++++++++++
>>  qmp-commands.hx  | 25 +++++++++++++++++++++++++
>>  savevm.c         | 34 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 81 insertions(+)

>> +
>> +Example (1):
>> +
>> +-> { "execute": "query-qdev-devices" }
>> +<- { "return": [
>> +       {
>> +         "devices": [ "kvm-tpr-opt", "piix4_pm" ]
> 
> Once here, can we change this to also include the device version?
> 
> i.e. something like:
> 
>  "devices": [ [ "device": [ "name": "kvm-tpr-opt", "version", 15]]], ...]

Not valid JSON, but see my other messages in the thread.  My idea of an
array of structs would be nice for this:

{ "return": [
  { "device": "kvm-tpr-opt", "version": 15 },
  { "device": "piix4_pm", "version": 1 }
] }

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH RFC v2 03/12] VMState test: query command to extract the qdevified device names

[Sanidhya Kashyap]

>> +##
>> +# @VMstatesQdevDevices
>> +#
>> +# list of qdevified devices that are registered with SaveStateEntry
>> +#
>> +# @device: list of qdevified device names
> 
> 
> Should we use qdev on the name?  Or just list of devices?  My
> understanding is that all devices are on this list, no?
> 

The problem is that there are some devices which have not been
qdevified. So, that's why I used the term qdev. If you want, I can
remove the term qdev. Sooner or later all the devices will be qdevified.

>> +
>> +-> { "execute": "query-qdev-devices" }
>> +<- { "return": [
>> +       {
>> +         "devices": [ "kvm-tpr-opt", "piix4_pm" ]
> 
> Once here, can we change this to also include the device version?
> 
> i.e. something like:
> 
>  "devices": [ [ "device": [ "name": "kvm-tpr-opt", "version", 15]]], ...]
> 
> Or somesuch?
> 

That is possible. Do you want any other information to be printed?

-- 
-----

Sanidhya Kashyap

[Qemu-devel] [PATCH RFC v2 04/12] VMState test: hmp interface for showing qdevified devices

[Sanidhya Kashyap]

This patch provides the hmp interface for qdevified devices list.

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 hmp-commands.hx |  2 ++
 hmp.c           | 21 +++++++++++++++++++++
 hmp.h           |  1 +
 monitor.c       |  7 +++++++
 4 files changed, 31 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d0943b1..4603de5 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1780,6 +1780,8 @@ show qdev device model list
 show roms
 @item info tpm
 show the TPM device
+@item info qdev_devices
+show the qdevified devices registered with migration capability
 @end table
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index 4d1838e..d1dd7d2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1714,3 +1714,24 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
 
     monitor_printf(mon, "\n");
 }
+
+void hmp_info_qdev_devices(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    VMStatesQdevDevices *qdev_devices = qmp_query_qdev_devices(&err);
+    strList *list = NULL;
+
+    if (qdev_devices) {
+        list = qdev_devices->device;
+        while (list) {
+            monitor_printf(mon, "%s\n", list->value);
+            list = list->next;
+        }
+    }
+
+    if (err) {
+        hmp_handle_error(mon, &err);
+    }
+
+    qapi_free_strList(list);
+}
diff --git a/hmp.h b/hmp.h
index 4fd3c4a..d179454 100644
--- a/hmp.h
+++ b/hmp.h
@@ -38,6 +38,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
 void hmp_info_pci(Monitor *mon, const QDict *qdict);
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
 void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_qdev_devices(Monitor *mon, const QDict *qdict);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index 5bc70a6..bf828d6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2918,6 +2918,13 @@ static mon_cmd_t info_cmds[] = {
         .mhandler.cmd = hmp_info_memdev,
     },
     {
+        .name       = "qdev_devices",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show registered qdevified devices",
+        .mhandler.cmd = hmp_info_qdev_devices,
+    },
+    {
         .name       = NULL,
     },
 };
-- 
1.9.3

[Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism

[Sanidhya Kashyap]

In this patch, I have made the following changes:

* changed the DPRINT statement.
* renamed the variables.
* added noqdev variable which decides which option to use for resetting.
* added devices option which can help in resetting one or many devices
(only qdevified ones).
* updated the documentation.

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 qapi-schema.json |  26 ++++++
 qmp-commands.hx  |  37 ++++++++
 savevm.c         | 251 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 314 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 996e6b5..ec48977 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3502,3 +3502,29 @@
 ##
 { 'command': 'query-qdev-devices',
   'returns': 'VMStatesQdevDevices' }
+
+##
+# @test-vmstates
+#
+# tests the vmstates' value by dumping and loading in memory
+#
+# @iterations: (optional) The total iterations for vmstate testing.
+# The min and max defind value is 10 and 100 respectively.
+#
+# @period: (optional) sleep interval between iteration (in milliseconds).
+# The default interval is 100 milliseconds with min and max being
+# 1 and 10000 respectively.
+#
+# @noqdev: boolean variable which decides whether to use qdevified devices
+# or not. Will be removed when all the devices have been qdevified.
+#
+# @devices: (optional) helps in resetting particular qdevified decices
+# that have been registered with SaveStateEntry
+#
+# Since 2.2
+##
+{ 'command': 'test-vmstates',
+  'data': {'*iterations': 'int',
+           '*period':     'int',
+           'noqdev':      'bool',
+           '*qdevices':   'VMStatesQdevDevices' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2e20032..6210f56 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3778,5 +3778,42 @@ Example (1):
        }
      ]
    }
+EQMP
+
+    {
+        .name       = "test-vmstates",
+        .args_type  = "iterations:i?,period:i?,noqdev:b,qdevices:O?",
+        .mhandler.cmd_new = qmp_marshal_input_test_vmstates,
+    },
+
+SQMP
+test-vmstates
+-------------
+
+Tests the vmstates' entry by dumping and loading in/from memory
+
+Arguments:
 
+- "iterations": (optional) The total iterations for vmstate testing.
+                 The min and max defined value is 10 and 100 respectively.
+
+- "period": (optional) sleep interval between iteration (in milliseconds).
+            The default interval is 100 milliseconds with min and max being
+            1 and 10000 respectively.
+
+- "noqdev": boolean variable which decides whether to use qdev or not.
+            Will be removed when all the devices have been qdevified.
+
+- "devices": (optional) helps in resetting particular qdevified decices
+                       that have been registered with SaveStateEntry
+
+
+Example:
+
+-> { "execute": "test-vmstates",
+     "arguments": {
+        "iterations": 10,
+        "period": 100,
+        "noqdev": false } }
+<- { "return": {} }
 EQMP
diff --git a/savevm.c b/savevm.c
index 7c1600a..304d8fc 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1201,6 +1201,257 @@ VMStatesQdevDevices *qmp_query_qdev_devices(Error **errp)
     return qdev_devices;
 }
 
+#define DEBUG_TEST_VMSTATES 1
+
+#ifndef DEBUG_TEST_VMSTATES
+#define DEBUG_TEST_VMSTATES 0
+#endif
+
+#define DPRINTF(fmt, ...) \
+    do { \
+        if (DEBUG_TEST_VMSTATES) { \
+            printf("vmstate_test: " fmt, ## __VA_ARGS__); \
+        } \
+    } while (0)
+
+#define TEST_VMSTATE_MIN_TIMES 10
+#define TEST_VMSTATE_MAX_TIMES 1000
+
+#define TEST_VMSTATE_MIN_INTERVAL_MS 1
+#define TEST_VMSTATE_DEFAULT_INTERVAL_MS 100
+#define TEST_VMSTATE_MAX_INTERVAL_MS 10000
+
+typedef struct VMStateLogState VMStateLogState;
+
+struct VMStateLogState {
+    int64_t current_iteration;
+    int64_t iterations;
+    int64_t period;
+    bool active_state;
+    bool noqdev;
+    VMStatesQdevDevices *qdevices;
+    QEMUTimer *timer;
+
+    QTAILQ_HEAD(qdev_list, VMStatesQdevResetEntry) qdev_list;
+};
+
+static VMStateLogState *vmstate_current_state(void)
+{
+    static VMStateLogState current_state = {
+        .active_state = false,
+    };
+
+    return &current_state;
+}
+
+static inline void test_vmstates_clear_qdev_entries(VMStateLogState *v)
+{
+    VMStatesQdevResetEntry *qre, *new_qre;
+    QTAILQ_FOREACH_SAFE(qre, &v->qdev_list, entry, new_qre) {
+        QTAILQ_REMOVE(&v->qdev_list, qre, entry);
+    }
+}
+
+static inline bool check_device_name(VMStateLogState *v,
+                                     VMStatesQdevDevices *qdevices,
+                                     Error **errp)
+{
+    VMStatesQdevResetEntry *qre;
+    strList *devices_name = qdevices->device;
+    QTAILQ_INIT(&v->qdev_list);
+    bool device_present;
+
+    /* now, checking against each one */
+    for (; devices_name; devices_name = devices_name->next) {
+        device_present = false;
+        VMStatesQdevResetEntry *new_qre;
+        QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) {
+            if (!strcmp(qre->device_name, devices_name->value)) {
+
+                device_present = true;
+
+                new_qre = g_malloc0(sizeof(VMStatesQdevResetEntry));
+                new_qre->dev = qre->dev;
+                strcpy(new_qre->device_name, qre->device_name);
+                QTAILQ_INSERT_TAIL(&v->qdev_list, new_qre, entry);
+
+                break;
+            }
+        }
+        if (!device_present) {
+            test_vmstates_clear_qdev_entries(v);
+            error_setg(errp, "Incorrect device name - %s\n",
+                       devices_name->value);
+            return false;
+        }
+    }
+    return true;
+}
+
+static inline void test_vmstates_reset_devices(VMStateLogState *v)
+{
+    VMStatesQdevResetEntry *qre;
+
+    if (v->noqdev) {
+        DPRINTF("resetting all devices\n");
+        qemu_system_reset(VMRESET_SILENT);
+    } else if (!v->qdevices) {
+        QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) {
+            DPRINTF("resetting device: %s\n", qre->device_name);
+            device_reset(qre->dev);
+        }
+    } else {
+        QTAILQ_FOREACH(qre, &v->qdev_list, entry) {
+            DPRINTF("resetting device: %s\n", qre->device_name);
+            device_reset(qre->dev);
+        }
+    }
+}
+
+static void vmstate_test_cb(void *opaque)
+{
+    VMStateLogState *v = opaque;
+    int saved_vm_running = runstate_is_running();
+    const QEMUSizedBuffer *qsb;
+    QEMUFile *f;
+    int ret;
+    int64_t save_vmstates_duration, load_vmstates_duration;
+    int64_t start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+
+    /* executing the steps for a single time with the help of timer */
+    if (++(v->current_iteration) <= v->iterations) {
+        saved_vm_running = runstate_is_running();
+
+        /* stopping the VM before dumping the vmstates */
+        vm_stop(RUN_STATE_SAVE_VM);
+
+        f = qemu_bufopen("w", NULL);
+        if (!f) {
+            goto testing_end;
+        }
+
+        cpu_synchronize_all_states();
+
+        /* saving the vmsates to memory buffer */
+        ret = qemu_save_device_state(f);
+        if (ret < 0) {
+            goto testing_end;
+        }
+        save_vmstates_duration = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) -
+                                 start_time;
+        DPRINTF("iteration: %ld, save time (ms): %ld\n",
+                v->current_iteration, save_vmstates_duration);
+
+        /* clearing the states of the guest */
+        test_vmstates_reset_devices(v);
+
+        start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+        qsb = qemu_buf_get(f);
+        f = qemu_bufopen("r", (QEMUSizedBuffer *)qsb);
+        if (!f) {
+            goto testing_end;
+        }
+
+        /* loading the device states from the saved buffer */
+        ret = qemu_loadvm_state(f);
+        qemu_fclose(f);
+        if (ret < 0) {
+            goto testing_end;
+        }
+        load_vmstates_duration = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) -
+                                 start_time;
+        DPRINTF("iteration: %ld, load time (ms): %ld\n",
+                v->current_iteration, load_vmstates_duration);
+
+        if (saved_vm_running) {
+            vm_start();
+        }
+        timer_mod(v->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
+                                              v->period);
+        return;
+    }
+
+ testing_end:
+    if (saved_vm_running) {
+        vm_start();
+    }
+    timer_del(v->timer);
+    timer_free(v->timer);
+    test_vmstates_clear_qdev_entries(v);
+    v->active_state = false;
+    return;
+}
+
+void qmp_test_vmstates(bool has_iterations, int64_t iterations,
+                       bool has_period, int64_t period, bool noqdev,
+                       bool has_qdevices, VMStatesQdevDevices *qdevices,
+                       Error **errp)
+{
+    VMStateLogState *v = vmstate_current_state();
+    Error *local_err;
+
+    if (v->active_state) {
+        error_setg(errp, "VMState testing already in progress\n");
+        return;
+    }
+
+    v->active_state = true;
+
+    /* checking the value of total iterations to be in the defined range */
+    if (!has_iterations) {
+        v->iterations = TEST_VMSTATE_MIN_TIMES;
+    } else if (iterations >= TEST_VMSTATE_MIN_TIMES &&
+               iterations <= TEST_VMSTATE_MAX_TIMES) {
+        v->iterations = iterations;
+    } else {
+        error_setg(errp, "iterations value must be in the range [%d, %d]\n",
+                   TEST_VMSTATE_MIN_TIMES, TEST_VMSTATE_MAX_TIMES);
+        v->active_state = false;
+        return;
+    }
+
+    /* checking for the value of period to be in the defined range */
+    if (!has_period) {
+        v->period = TEST_VMSTATE_DEFAULT_INTERVAL_MS;
+    } else if (period >= TEST_VMSTATE_MIN_INTERVAL_MS &&
+               period <= TEST_VMSTATE_MAX_INTERVAL_MS) {
+        v->period = period;
+    } else {
+        error_setg(errp, "sleep interval (period) value must be "
+                   "in the defined range [%d, %d](ms)\n",
+                   TEST_VMSTATE_MIN_INTERVAL_MS, TEST_VMSTATE_MAX_INTERVAL_MS);
+        v->active_state = false;
+        return;
+    }
+
+    /*
+     * checking the devices information
+     * if no devices have been selected, then all the devices will be tested
+     * noqdev -> if true -- then use qemu_system_reset
+     *        -> if false -- then use qdevified devices
+     */
+    if (noqdev && has_qdevices) {
+        error_setg(errp, "either qdev or non-qdev devices allowed, not both\n");
+        return;
+    } else if (!noqdev && !has_qdevices) {
+        v->qdevices = NULL;
+    } else if (has_qdevices) {
+        if (check_device_name(v, qdevices, &local_err)) {
+            v->qdevices = qdevices;
+        } else {
+            if (local_err) {
+                error_propagate(errp, local_err);
+            }
+            return;
+        }
+    }
+
+    v->noqdev = noqdev;
+    v->current_iteration = 0;
+    v->timer = timer_new_ms(QEMU_CLOCK_REALTIME, vmstate_test_cb, v);
+    timer_mod(v->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
+}
+
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism

[Eric Blake]

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

On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
> In this patch, I have made the following changes:
> 
> * changed the DPRINT statement.
> * renamed the variables.
> * added noqdev variable which decides which option to use for resetting.
> * added devices option which can help in resetting one or many devices
> (only qdevified ones).
> * updated the documentation.
> 
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---
>  qapi-schema.json |  26 ++++++
>  qmp-commands.hx  |  37 ++++++++
>  savevm.c         | 251 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 314 insertions(+)
> 
> +#
> +# @noqdev: boolean variable which decides whether to use qdevified devices
> +# or not. Will be removed when all the devices have been qdevified.

Please name this 'x-noqdev' if it is likely to be removed in the future,
to make it obvious that it is not part of the permanent API.  Also, mark
it as optional, with a sane default, so that you can test the API
without being forced to supply this temporary parameter.

> +#
> +# @devices: (optional) helps in resetting particular qdevified decices
> +# that have been registered with SaveStateEntry
> +#
> +# Since 2.2
> +##
> +{ 'command': 'test-vmstates',
> +  'data': {'*iterations': 'int',
> +           '*period':     'int',
> +           'noqdev':      'bool',
> +           '*qdevices':   'VMStatesQdevDevices' } }

Based on my comments on 3/12, if you rename that qapi to:

{ 'type': 'VMStatesQdevDevice',
  'data': { 'device': 'str' } }

then this should be

'*qdevices': [ 'VMStatesQdevDevice' ]


> +
> +- "devices": (optional) helps in resetting particular qdevified decices

s/decices/devices/

> +                       that have been registered with SaveStateEntry
> +
> +
> +Example:
> +
> +-> { "execute": "test-vmstates",
> +     "arguments": {
> +        "iterations": 10,
> +        "period": 100,
> +        "noqdev": false } }

If noqdev is going to disappear, I wouldn't include it in the example.
Conversely, showing how to use 'devices' might be useful.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism

[Juan Quintela]

Sanidhya Kashyap <sanidhya.iiith@gmail.com> wrote:
> In this patch, I have made the following changes:
>
> * changed the DPRINT statement.
> * renamed the variables.
> * added noqdev variable which decides which option to use for resetting.
> * added devices option which can help in resetting one or many devices
> (only qdevified ones).
> * updated the documentation.
>
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>

> +##
> +# @test-vmstates
> +#
> +# tests the vmstates' value by dumping and loading in memory
> +#
> +# @iterations: (optional) The total iterations for vmstate testing.
> +# The min and max defind value is 10 and 100 respectively.
> +#
> +# @period: (optional) sleep interval between iteration (in milliseconds).
> +# The default interval is 100 milliseconds with min and max being
> +# 1 and 10000 respectively.
> +#
> +# @noqdev: boolean variable which decides whether to use qdevified devices
> +# or not. Will be removed when all the devices have been qdevified.
> +#
> +# @devices: (optional) helps in resetting particular qdevified decices
> +# that have been registered with SaveStateEntry
> +#
> +# Since 2.2
> +##
> +{ 'command': 'test-vmstates',
> +  'data': {'*iterations': 'int',
> +           '*period':     'int',
> +           'noqdev':      'bool',

Do we really care about "noqdev", or should we just "decree" that it is
"false" always?


> +#define DEBUG_TEST_VMSTATES 1
> +
> +#ifndef DEBUG_TEST_VMSTATES
> +#define DEBUG_TEST_VMSTATES 0
> +#endif

If you have the previe line, this ones are not needed.


> +
> +#define DPRINTF(fmt, ...) \
> +    do { \
> +        if (DEBUG_TEST_VMSTATES) { \
> +            printf("vmstate_test: " fmt, ## __VA_ARGS__); \
> +        } \
> +    } while (0)

DPRINTF is *so* last year O:-)
It is considedered better to just add tracepoints to the code.  I think
that all the DPRINTF's make sense to be a tracepoint, no?


> +struct VMStateLogState {
> +    int64_t current_iteration;
> +    int64_t iterations;
> +    int64_t period;
> +    bool active_state;
> +    bool noqdev;
> +    VMStatesQdevDevices *qdevices;
> +    QEMUTimer *timer;
> +
> +    QTAILQ_HEAD(qdev_list, VMStatesQdevResetEntry) qdev_list;
> +};
> +
> +static VMStateLogState *vmstate_current_state(void)
> +{
> +    static VMStateLogState current_state = {
> +        .active_state = false,
> +    };
> +
> +    return &current_state;
> +}
> +
> +static inline void test_vmstates_clear_qdev_entries(VMStateLogState *v)

We need a better preffix that test_vmstates_
But I can't think of one right now.  Will think later about it.


> +static inline bool check_device_name(VMStateLogState *v,
> +                                     VMStatesQdevDevices *qdevices,
> +                                     Error **errp)

Is "inline" needed?  I would expect the compiler to do a reasonable
decision with an static function called only once?

> +{
> +    VMStatesQdevResetEntry *qre;
> +    strList *devices_name = qdevices->device;
> +    QTAILQ_INIT(&v->qdev_list);
> +    bool device_present;
> +
> +    /* now, checking against each one */
> +    for (; devices_name; devices_name = devices_name->next) {
> +        device_present = false;
> +        VMStatesQdevResetEntry *new_qre;
> +        QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) {
> +            if (!strcmp(qre->device_name, devices_name->value)) {
> +
> +                device_present = true;
> +
> +                new_qre = g_malloc0(sizeof(VMStatesQdevResetEntry));
> +                new_qre->dev = qre->dev;
> +                strcpy(new_qre->device_name, qre->device_name);
> +                QTAILQ_INSERT_TAIL(&v->qdev_list, new_qre, entry);
> +
> +                break;
                   return;

And remove the whole "device_present" variable and assignation?

> +            }
> +        }
> +        if (!device_present) {
> +            test_vmstates_clear_qdev_entries(v);
> +            error_setg(errp, "Incorrect device name - %s\n",
> +                       devices_name->value);
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +
> +static inline void test_vmstates_reset_devices(VMStateLogState *v)
> +{
> +    VMStatesQdevResetEntry *qre;
> +
> +    if (v->noqdev) {
> +        DPRINTF("resetting all devices\n");
> +        qemu_system_reset(VMRESET_SILENT);

What is diffe9rent between calling with "noqdev" and with an empty
device list?  I would expect them to be the same list of devices.

> +    } else if (!v->qdevices) {
> +        QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) {
> +            DPRINTF("resetting device: %s\n", qre->device_name);
> +            device_reset(qre->dev);
> +        }
> +    } else {
> +        QTAILQ_FOREACH(qre, &v->qdev_list, entry) {
> +            DPRINTF("resetting device: %s\n", qre->device_name);
> +            device_reset(qre->dev);
> +        }
> +    }
> +}
> +
> +static void vmstate_test_cb(void *opaque)
> +{
> +    VMStateLogState *v = opaque;
> +    int saved_vm_running = runstate_is_running();
> +    const QEMUSizedBuffer *qsb;
> +    QEMUFile *f;
> +    int ret;
> +    int64_t save_vmstates_duration, load_vmstates_duration;
> +    int64_t start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +
> +    /* executing the steps for a single time with the help of timer */
> +    if (++(v->current_iteration) <= v->iterations) {
> +        saved_vm_running = runstate_is_running();
> +
> +        /* stopping the VM before dumping the vmstates */
> +        vm_stop(RUN_STATE_SAVE_VM);
> +
> +        f = qemu_bufopen("w", NULL);
> +        if (!f) {
> +            goto testing_end;
> +        }

I think we can call qemu_bufopen() out of the timer, and then doing the
free on the cleanup?


> +
> +        cpu_synchronize_all_states();
> +
> +        /* saving the vmsates to memory buffer */
> +        ret = qemu_save_device_state(f);
> +        if (ret < 0) {
> +            goto testing_end;
> +        }
> +        save_vmstates_duration = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) -
> +                                 start_time;
> +        DPRINTF("iteration: %ld, save time (ms): %ld\n",
> +                v->current_iteration, save_vmstates_duration);
> +
> +        /* clearing the states of the guest */
> +        test_vmstates_reset_devices(v);
> +
> +        start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +        qsb = qemu_buf_get(f);
> +        f = qemu_bufopen("r", (QEMUSizedBuffer *)qsb);

We are only using this function once, can't we convince it to just be
called "const"?


ok what are we doing here:


for(i=0; i< times; i++) {
       .....
       f = qemu_bufopen("r", ..);
       .....
       f = qemu_buf_get(f);
       f = qemu_bufopen("w", ..)
       ...
       qemu_fclose(f);
}


What I propose is switching to something like:

f = qemu_bufopen("r", ..);

for(i=0; i< times; i++) {
       ....
       qemu_buf_set_ro(f);
       .....
       qemu_buf_set_rw(f)
       ...
}
qemu_fclose(f);


This makes qemu_bufopen() way simpler.  Once there, my understanding is
that current code is leaking a QEMUBuffer each time that we call
     qemu_bufopen("w", ...)


> +    if (!has_period) {
> +        v->period = TEST_VMSTATE_DEFAULT_INTERVAL_MS;
> +    } else if (period >= TEST_VMSTATE_MIN_INTERVAL_MS &&
> +               period <= TEST_VMSTATE_MAX_INTERVAL_MS) {
> +        v->period = period;
> +    } else {
> +        error_setg(errp, "sleep interval (period) value must be "
> +                   "in the defined range [%d, %d](ms)\n",
> +                   TEST_VMSTATE_MIN_INTERVAL_MS, TEST_VMSTATE_MAX_INTERVAL_MS);
> +        v->active_state = false;
> +        return;
> +    }

I think this sholud be a macro and not being repeated by each numeric
parameter, but that is a QMP API, not related to this patch.

Thanks, Juan.

Re: [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism

[Sanidhya Kashyap]

>> +{ 'command': 'test-vmstates',
>> +  'data': {'*iterations': 'int',
>> +           '*period':     'int',
>> +           'noqdev':      'bool',
> 
> Do we really care about "noqdev", or should we just "decree" that it is
> "false" always?
> 

Okay. Will remove it.

> 
>> +#define DEBUG_TEST_VMSTATES 1
>> +
>> +#ifndef DEBUG_TEST_VMSTATES
>> +#define DEBUG_TEST_VMSTATES 0
>> +#endif
> 
> If you have the previe line, this ones are not needed.
> 
> 
>> +
>> +#define DPRINTF(fmt, ...) \
>> +    do { \
>> +        if (DEBUG_TEST_VMSTATES) { \
>> +            printf("vmstate_test: " fmt, ## __VA_ARGS__); \
>> +        } \
>> +    } while (0)
> 
> DPRINTF is *so* last year O:-)
> It is considedered better to just add tracepoints to the code.  I think
> that all the DPRINTF's make sense to be a tracepoint, no?
> 
>

yup, tracepoints do make sense. Will incorporate that.

> We need a better preffix that test_vmstates_
> But I can't think of one right now.  Will think later about it.
> 
> 

I am really bad with naming conventions :-/. Whatever seems fit to you.
I'll use that.

>> +static inline bool check_device_name(VMStateLogState *v,
>> +                                     VMStatesQdevDevices *qdevices,
>> +                                     Error **errp)
> 
> Is "inline" needed?  I would expect the compiler to do a reasonable
> decision with an static function called only once?
> 

My mistake. Will correct it.

>> +{
>> +    VMStatesQdevResetEntry *qre;
>> +    strList *devices_name = qdevices->device;
>> +    QTAILQ_INIT(&v->qdev_list);
>> +    bool device_present;
>> +
>> +    /* now, checking against each one */
>> +    for (; devices_name; devices_name = devices_name->next) {
>> +        device_present = false;
>> +        VMStatesQdevResetEntry *new_qre;
>> +        QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) {
>> +            if (!strcmp(qre->device_name, devices_name->value)) {
>> +
>> +                device_present = true;
>> +
>> +                new_qre = g_malloc0(sizeof(VMStatesQdevResetEntry));
>> +                new_qre->dev = qre->dev;
>> +                strcpy(new_qre->device_name, qre->device_name);
>> +                QTAILQ_INSERT_TAIL(&v->qdev_list, new_qre, entry);
>> +
>> +                break;
>                    return;
> 
> And remove the whole "device_present" variable and assignation?
> 

Actually, I have used this variable which will help us in deciding
whether the user has provided a sane list of vmstates name's or not. If
the names do not match, then we do not proceed. That is why I have used
the device_present variable. I'll change the variable name.

>> +    if (v->noqdev) {
>> +        DPRINTF("resetting all devices\n");
>> +        qemu_system_reset(VMRESET_SILENT);
> 
> What is diffe9rent between calling with "noqdev" and with an empty
> device list?  I would expect them to be the same list of devices.
> 

Well, they are not. Currently, when qemu_system_reset() is called the
mc->reset is NULL. So, the old way of resetting the device takes place
which has some different devices or might be bus registered. Therefore,
I have tried to provide whatever is there on the table i.e related to
the resetting. But, that is not required, I'll completely remove it.

>> +        f = qemu_bufopen("w", NULL);
>> +        if (!f) {
>> +            goto testing_end;
>> +        }
> 
> I think we can call qemu_bufopen() out of the timer, and then doing the
> free on the cleanup?
> 
> 

If I perform the cleanup at the end, then I will be eating the memory.
When I close the buffer, at least that memory is freed. The other issue
will be taking the read pointer back to the write pointer, of which I
don't know whether there is a support or not.

>> +
>> +        cpu_synchronize_all_states();
>> +
>> +        /* saving the vmsates to memory buffer */
>> +        ret = qemu_save_device_state(f);
>> +        if (ret < 0) {
>> +            goto testing_end;
>> +        }
>> +        save_vmstates_duration = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) -
>> +                                 start_time;
>> +        DPRINTF("iteration: %ld, save time (ms): %ld\n",
>> +                v->current_iteration, save_vmstates_duration);
>> +
>> +        /* clearing the states of the guest */
>> +        test_vmstates_reset_devices(v);
>> +
>> +        start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>> +        qsb = qemu_buf_get(f);
>> +        f = qemu_bufopen("r", (QEMUSizedBuffer *)qsb);
> 
> We are only using this function once, can't we convince it to just be
> called "const"?
> 
> 
> ok what are we doing here:
> 
> 
> for(i=0; i< times; i++) {
>        .....
>        f = qemu_bufopen("r", ..);
>        .....
>        f = qemu_buf_get(f);
>        f = qemu_bufopen("w", ..)
>        ...
>        qemu_fclose(f);
> }
> 
> 
> What I propose is switching to something like:
> 
> f = qemu_bufopen("r", ..);
> 
> for(i=0; i< times; i++) {
>        ....
>        qemu_buf_set_ro(f);
>        .....
>        qemu_buf_set_rw(f)
>        ...
> }
> qemu_fclose(f);
> 
> 
> This makes qemu_bufopen() way simpler.  Once there, my understanding is
> that current code is leaking a QEMUBuffer each time that we call
>      qemu_bufopen("w", ...)
> 
> 

Yup, I have missed qemu_fclose(f) before

f = qemu_bufopen("r", ..);

I'll correct it. Now, regarding the qemu_buf_set_ro and qemu_buf_set_rw,
I guess, I'll have to rewind the pointer, for which I have to get some
idea before doing it, or extend the QEMUFile code for memory buffer.


-- 
-----

Sanidhya Kashyap

[Qemu-devel] [PATCH RFC v2 06/12] VMState test: hmp interface for vmstate testing

[Sanidhya Kashyap]

I have added the hmp interface for vmstate testing. Currently, the patch does
not support the qdev list, since I could not figure out how to the pass the
VMStatesQdevDevices struct which can be parsed and used.

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 hmp-commands.hx | 15 +++++++++++++++
 hmp.c           | 18 ++++++++++++++++++
 hmp.h           |  1 +
 3 files changed, 34 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 4603de5..6af72a6 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1790,6 +1790,21 @@ STEXI
 show available trace events and their state
 ETEXI
 
+     {
+        .name       = "test-vmstates",
+        .args_type  = "iterations:i?,period:i?",
+        .params     = "total_iterations sleep_interval",
+        .help       = "test the vmstates by dumping and loading form memory\n\t\t\t"
+                      "iterations: (optional) number of times, the vmstates will be tested\n\t\t\t"
+                      "period: (optional) sleep interval in milliseconds between each iteration",
+        .mhandler.cmd = hmp_test_vmstates,
+    },
+STEXI
+@item test-vmstates
+@findex test-vmstates
+dumps and reads the device state's data from the memory for testing purpose
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index d1dd7d2..6c998d2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1735,3 +1735,21 @@ void hmp_info_qdev_devices(Monitor *mon, const QDict *qdict)
 
     qapi_free_strList(list);
 }
+
+void hmp_test_vmstates(Monitor *mon, const QDict *qdict)
+{
+    int has_iterations = qdict_haskey(qdict, "iterations");
+    int64_t iterations = qdict_get_try_int(qdict, "iterations", 10);
+    int has_period = qdict_haskey(qdict, "period");
+    int64_t period = qdict_get_try_int(qdict, "period", 100);
+
+    Error *err = NULL;
+
+    qmp_test_vmstates(has_iterations, iterations, has_period, period,
+                      true, false, NULL, &err);
+
+    if (err) {
+        monitor_printf(mon, "test-vmstates: %s\n", error_get_pretty(err));
+        error_free(err);
+    }
+}
diff --git a/hmp.h b/hmp.h
index d179454..41bc781 100644
--- a/hmp.h
+++ b/hmp.h
@@ -95,6 +95,7 @@ void hmp_cpu_add(Monitor *mon, const QDict *qdict);
 void hmp_object_add(Monitor *mon, const QDict *qdict);
 void hmp_object_del(Monitor *mon, const QDict *qdict);
 void hmp_info_memdev(Monitor *mon, const QDict *qdict);
+void hmp_test_vmstates(Monitor *mon, const QDict *qdict);
 void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
 void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
 void device_add_completion(ReadLineState *rs, int nb_args, const char *str);
-- 
1.9.3

[Qemu-devel] [PATCH RFC v2 07/12] VMState test: qmp interface for querying the vmstate testing process

[Sanidhya Kashyap]

This patch provides the information about an already executing testing
process. I have modified the qmp command to query-test-vmstates from
test-vmstates-get-info.

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 qapi-schema.json | 34 ++++++++++++++++++++++++++++++++++
 qmp-commands.hx  | 25 +++++++++++++++++++++++++
 savevm.c         | 18 ++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index ec48977..a12872f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3528,3 +3528,37 @@
            '*period':     'int',
            'noqdev':      'bool',
            '*qdevices':   'VMStatesQdevDevices' } }
+
+##
+# @VMStateLogStateInfo
+#
+# VMState testing information
+# Tells about the current iteration, the total iterations
+# that have been provided and the sleep interval
+#
+# @current-iteration: shows the current iteration at which
+# the test is in.
+#
+# @iterations: the allocated total iterations for the vmstate
+# testing process.
+#
+# @period: the allowed sleep interval between each iteration
+#          (in milliseconds).
+#
+# Since 2.2
+##
+{ 'type': 'VMStateLogStateInfo',
+  'data': { 'current-iteration': 'int',
+            'iterations':        'int',
+            'period':            'int' } }
+
+##
+# @query-test-vmstates
+#
+# Get the current parameters value of the vmstate testing process.
+#
+# Returns VMStateLogStateInfo structure.
+#
+# Since 2.2
+##
+{ 'command': 'query-test-vmstates', 'returns': 'VMStateLogStateInfo' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6210f56..0a40a88 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3817,3 +3817,28 @@ Example:
         "noqdev": false } }
 <- { "return": {} }
 EQMP
+
+    {
+        .name       = "query-test-vmstates",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_test_vmstates,
+    },
+
+SQMP
+query-test-vmstates-info
+------------------------
+
+Get the parameters information
+
+- "current_iteration": the current iteration going on
+- "iterations:" the total number of assigned iterations
+- "period": the sleep interval between the iteration
+
+Example:
+
+-> { "execute": "query-test-vmstates" }
+<- { "return": {
+            "current_iteration": 3,
+            "iterations": 100,
+            "period": 100 } }
+EQMP
diff --git a/savevm.c b/savevm.c
index b5e53b8..793fee7 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1451,6 +1451,24 @@ void qmp_test_vmstates(bool has_iterations, int64_t iterations,
     timer_mod(v->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
 }
 
+VMStateLogStateInfo *qmp_query_test_vmstates(Error **errp)
+{
+    VMStateLogState *v = vmstate_current_state();
+    VMStateLogStateInfo *log_info = NULL;
+
+    if (!v->active_state) {
+        return log_info;
+    }
+
+    log_info = g_malloc0(sizeof(VMStateLogStateInfo));
+
+    log_info->current_iteration = v->current_iteration;
+    log_info->iterations = v->iterations;
+    log_info->period = v->period;
+
+    return log_info;
+}
+
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC v2 07/12] VMState test: qmp interface for querying the vmstate testing process

[Eric Blake]

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

On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
> This patch provides the information about an already executing testing
> process. I have modified the qmp command to query-test-vmstates from
> test-vmstates-get-info.
> 
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---
>  qapi-schema.json | 34 ++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  | 25 +++++++++++++++++++++++++
>  savevm.c         | 18 ++++++++++++++++++
>  3 files changed, 77 insertions(+)
> 

> +##
> +{ 'type': 'VMStateLogStateInfo',
> +  'data': { 'current-iteration': 'int',
> +            'iterations':        'int',
> +            'period':            'int' } }

Looks okay...


> +++ b/qmp-commands.hx

> +SQMP
> +query-test-vmstates-info
> +------------------------
> +
> +Get the parameters information
> +
> +- "current_iteration": the current iteration going on

s/_/-/

> +- "iterations:" the total number of assigned iterations
> +- "period": the sleep interval between the iteration
> +
> +Example:
> +
> +-> { "execute": "query-test-vmstates" }
> +<- { "return": {
> +            "current_iteration": 3,

...but spelling doesn't match the actual code.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH RFC v2 07/12] VMState test: qmp interface for querying the vmstate testing process

[Eric Blake]

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

On 07/29/2014 09:17 AM, Eric Blake wrote:
> On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
>> This patch provides the information about an already executing testing
>> process. I have modified the qmp command to query-test-vmstates from
>> test-vmstates-get-info.

This last sentence does not belong in the commit message proper, rather,
move it...

>>
>> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
>> ---

...here, as a changelog for how the v2 differs from v1.  Years from now,
when someone is reading qemu.git, they won't care what alternative names
were proposed, only what is actually committed.

> 
>> +++ b/qmp-commands.hx
> 
>> +SQMP
>> +query-test-vmstates-info
>> +------------------------

s/-info//, to match the actual command name that you documented in the
.json.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH RFC v2 08/12] VMState test: hmp interface for querying the vmstate testing process

[Sanidhya Kashyap]

Added a hmp interface for providing the information about the testing process.
I have used the underscore as a separater on Eric's advice. But, I have found
some of the commands having hyphen.

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 hmp-commands.hx |  2 ++
 hmp.c           | 14 ++++++++++++++
 hmp.h           |  1 +
 monitor.c       |  7 +++++++
 4 files changed, 24 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 6af72a6..c1dc6a2 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1770,6 +1770,8 @@ show migration status
 show current migration capabilities
 @item info migrate_cache_size
 show current migration XBZRLE cache size
+@item info test_vmstates
+show current vmstates testing process info
 @item info balloon
 show balloon information
 @item info qtree
diff --git a/hmp.c b/hmp.c
index 9e01127..385fb99 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1753,3 +1753,17 @@ void hmp_test_vmstates(Monitor *mon, const QDict *qdict)
         error_free(err);
     }
 }
+
+void hmp_info_test_vmstates(Monitor *mon, const QDict *qdict)
+{
+    VMStateLogStateInfo *log_info = qmp_query_test_vmstates(NULL);
+
+    if (log_info) {
+        monitor_printf(mon, "current-iteration: %"PRId64 "\n"
+                            "iterations: %"PRId64 "\n"
+                            "period: %"PRId64 "\n", log_info->current_iteration,
+                            log_info->iterations, log_info->period);
+    }
+
+    qapi_free_VMStateLogStateInfo(log_info);
+}
diff --git a/hmp.h b/hmp.h
index 41bc781..b77f14c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -39,6 +39,7 @@ void hmp_info_pci(Monitor *mon, const QDict *qdict);
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
 void hmp_info_tpm(Monitor *mon, const QDict *qdict);
 void hmp_info_qdev_devices(Monitor *mon, const QDict *qdict);
+void hmp_info_test_vmstates(Monitor *mon, const QDict *qdict);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index bf828d6..427eef1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2862,6 +2862,13 @@ static mon_cmd_t info_cmds[] = {
         .mhandler.cmd = hmp_info_migrate_capabilities,
     },
     {
+        .name       = "test_vmstates",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show current vmstates testing process info",
+        .mhandler.cmd = hmp_info_test_vmstates,
+    },
+    {
         .name       = "migrate_cache_size",
         .args_type  = "",
         .params     = "",
-- 
1.9.3

[Qemu-devel] [PATCH RFC v2 09/12] VMState test: update period of vmstate testing process

[Sanidhya Kashyap]

No particular change, except variable name. Since I am not modifying other
variables, so I have not made the command generic.

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 qapi-schema.json | 12 ++++++++++++
 qmp-commands.hx  | 23 +++++++++++++++++++++++
 savevm.c         | 13 +++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index a12872f..13e922e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3562,3 +3562,15 @@
 # Since 2.2
 ##
 { 'command': 'query-test-vmstates', 'returns': 'VMStateLogStateInfo' }
+
+##
+# @test-vmstates-set-period
+#
+# sets the sleep interval between iterations of the vmstate testing process
+#
+# @period: the updated sleep interval value (in milliseconds)
+#
+# Since 2.2
+##
+{ 'command' : 'test-vmstates-set-period',
+  'data'    : { 'period': 'int' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0a40a88..2f019b0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3842,3 +3842,26 @@ Example:
             "iterations": 100,
             "period": 100 } }
 EQMP
+
+    {
+        .name       = "test-vmstates-set-period",
+        .args_type  = "period:i",
+        .mhandler.cmd_new = qmp_marshal_input_test_vmstates_set_period,
+    },
+
+SQMP
+test-vmstates-set-period
+------------------------
+
+Update the sleep interval for the remaining iterations
+
+Arguments:
+
+- "period": the updated sleep interval between iterations (json-int)
+
+Example:
+
+-> { "execute": "test-vmstates-set-period", "arguments": { "period": 1024 } }
+<- { "return": {} }
+EQMP
+
diff --git a/savevm.c b/savevm.c
index 797be57..d5fb93b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1470,6 +1470,19 @@ VMStateLogStateInfo *qmp_query_test_vmstates(Error **errp)
     return log_info;
 }
 
+void qmp_test_vmstates_set_period(int64_t period, Error **errp)
+{
+    VMStateLogState *v = vmstate_current_state();
+    if (period < TEST_VMSTATE_MIN_INTERVAL_MS ||
+        period > TEST_VMSTATE_MAX_INTERVAL_MS) {
+        error_setg(errp, "sleep interval (period) value must be "
+                   "in the defined range [%d, %d](ms)\n",
+                   TEST_VMSTATE_MIN_INTERVAL_MS, TEST_VMSTATE_MAX_INTERVAL_MS);
+        return;
+    }
+    v->period = period;
+}
+
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC v2 09/12] VMState test: update period of vmstate testing process

[Eric Blake]

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

On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
> No particular change, except variable name. Since I am not modifying other
> variables, so I have not made the command generic.

This sentence feels like changelog information compared to v1; as such,
it belongs...

> 
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---

...here, for use in the mail but not part of the commit itself.  On the
other hand, the commit itself should mention the name of the QMP command
it is adding (I shouldn't have to read the body of the patch to see what
the command name is).

>  qapi-schema.json | 12 ++++++++++++
>  qmp-commands.hx  | 23 +++++++++++++++++++++++
>  savevm.c         | 13 +++++++++++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a12872f..13e922e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3562,3 +3562,15 @@
>  # Since 2.2
>  ##
>  { 'command': 'query-test-vmstates', 'returns': 'VMStateLogStateInfo' }
> +
> +##
> +# @test-vmstates-set-period
> +#
> +# sets the sleep interval between iterations of the vmstate testing process
> +#
> +# @period: the updated sleep interval value (in milliseconds)
> +#
> +# Since 2.2
> +##
> +{ 'command' : 'test-vmstates-set-period',
> +  'data'    : { 'period': 'int' } }

Is it possible that we might add other tunables in the future?  If so,
this command is not very scalable (we would be adding one command per
new tunable).  On the other hand, adding complexity to achieve a generic
setter is not worth it if we only expect one tunable.

> +void qmp_test_vmstates_set_period(int64_t period, Error **errp)
> +{
> +    VMStateLogState *v = vmstate_current_state();
> +    if (period < TEST_VMSTATE_MIN_INTERVAL_MS ||
> +        period > TEST_VMSTATE_MAX_INTERVAL_MS) {
> +        error_setg(errp, "sleep interval (period) value must be "
> +                   "in the defined range [%d, %d](ms)\n",
> +                   TEST_VMSTATE_MIN_INTERVAL_MS, TEST_VMSTATE_MAX_INTERVAL_MS);
> +        return;
> +    }
> +    v->period = period;
> +}

This looks like it takes effect whether or not a vmstate test is
underway.  Does this impact the default of the next vmstate test to
start, in the case where that command doesn't supply a value for period
but relies on the default?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH RFC v2 09/12] VMState test: update period of vmstate testing process

[Sanidhya Kashyap]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

>> +# +# Since 2.2 +## +{ 'command' : 'test-vmstates-set-period', +
>> 'data'    : { 'period': 'int' } }
> 
> Is it possible that we might add other tunables in the future?  If
> so, this command is not very scalable (we would be adding one
> command per new tunable).  On the other hand, adding complexity to
> achieve a generic setter is not worth it if we only expect one
> tunable.
> 


I was thinking of making it scalable as you already pointed out in the
previous patch but it does not look like there is something going to
be added in the future. So, that's why I am stuck with a single
parameter change.

>> +void qmp_test_vmstates_set_period(int64_t period, Error **errp) 
>> +{ +    VMStateLogState *v = vmstate_current_state(); +    if
>> (period < TEST_VMSTATE_MIN_INTERVAL_MS || +        period >
>> TEST_VMSTATE_MAX_INTERVAL_MS) { +        error_setg(errp, "sleep
>> interval (period) value must be " +                   "in the
>> defined range [%d, %d](ms)\n", +
>> TEST_VMSTATE_MIN_INTERVAL_MS, TEST_VMSTATE_MAX_INTERVAL_MS); +
>> return; +    } +    v->period = period; +}
> 
> This looks like it takes effect whether or not a vmstate test is 
> underway.  Does this impact the default of the next vmstate test
> to

Yes

> start, in the case where that command doesn't supply a value for
> period but relies on the default?

No, it does not.


- -- 
- -----

Sanidhya Kashyap
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJT1+JFAAoJEFt9RLmoahln4qcIAJngjCqqQtDCjuAPnGxQnamM
Y7IUNe9Z0fGIYp88tHZN32cpaxbMkT+BEL7QeJDynrXcaH+SVIwOepPQNYqVdzzh
q4ldQW/D5VxC/xKXtenHZbAx3+Iat00dHHutq8y4ezR1DH9N1GfTHyKCewP6WP96
dy4RAJvKgItpRfu3FuGoLRxqWbAre1NrwydQZtiv3C1t14sM/Ua2j7Syp5vhhCtM
GtQrhKt2I1Hr0fGupbarLWRNKXyCBi7XgDG6+NvJNP6ECAh5GeVmQ7e9L891tL3b
wi9a4GNL45sVLa3tLBG81aGIn1XJOX6UEdRJIqWyIKCkC/LS417waitb1oWWfYw=
=9HHf
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH RFC v2 09/12] VMState test: update period of vmstate testing process

[Eric Blake]

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

On 07/29/2014 12:04 PM, Sanidhya Kashyap wrote:

>>> +void qmp_test_vmstates_set_period(int64_t period, Error **errp) 
>>> +{ +    VMStateLogState *v = vmstate_current_state(); +    if
>>> (period < TEST_VMSTATE_MIN_INTERVAL_MS || +        period >
>>> TEST_VMSTATE_MAX_INTERVAL_MS) { +        error_setg(errp, "sleep
>>> interval (period) value must be " +                   "in the
>>> defined range [%d, %d](ms)\n", +
>>> TEST_VMSTATE_MIN_INTERVAL_MS, TEST_VMSTATE_MAX_INTERVAL_MS); +
>>> return; +    } +    v->period = period; +}

Uggh, your mailer clobbered whitespace when replying :(

> 
>> This looks like it takes effect whether or not a vmstate test is 
>> underway.  Does this impact the default of the next vmstate test
>> to
> 
> Yes
> 
>> start, in the case where that command doesn't supply a value for
>> period but relies on the default?
> 
> No, it does not.

Then it may be worth fixing the command to error out if there is no
active vmstate test underway, instead of changing a value that has no
effect.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH RFC v2 10/12] VMState test: hmp interface for period update

[Sanidhya Kashyap]

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 hmp-commands.hx | 15 +++++++++++++++
 hmp.c           | 14 ++++++++++++++
 hmp.h           |  1 +
 3 files changed, 30 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index c1dc6a2..6d15184 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1807,6 +1807,21 @@ STEXI
 dumps and reads the device state's data from the memory for testing purpose
 ETEXI
 
+    {
+        .name       = "test_vmstates_set_period",
+        .args_type  = "period:i",
+        .params     = "period",
+        .help       = "set the sleep interval for vmstates testing process\n\t\t\t"
+                      "period: the new sleep interval value to replace the existing",
+        .mhandler.cmd = hmp_test_vmstates_set_period,
+    },
+
+STEXI
+@item test_vmstates_set_period @var{period}
+@findex test_vmstates_set_period
+Set the period to @var{period} (int) for vmstate testing process.
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index 385fb99..f54b0b9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1767,3 +1767,17 @@ void hmp_info_test_vmstates(Monitor *mon, const QDict *qdict)
 
     qapi_free_VMStateLogStateInfo(log_info);
 }
+
+void hmp_test_vmstates_set_period(Monitor *mon, const QDict *qdict)
+{
+    int64_t period = qdict_get_int(qdict, "period");
+    Error *err = NULL;
+
+    qmp_test_vmstates_set_period(period, &err);
+
+    if (err) {
+        monitor_printf(mon, "test-vmstates-set-period: %s\n",
+                       error_get_pretty(err));
+        error_free(err);
+    }
+}
diff --git a/hmp.h b/hmp.h
index b77f14c..e1afde8 100644
--- a/hmp.h
+++ b/hmp.h
@@ -97,6 +97,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict);
 void hmp_object_del(Monitor *mon, const QDict *qdict);
 void hmp_info_memdev(Monitor *mon, const QDict *qdict);
 void hmp_test_vmstates(Monitor *mon, const QDict *qdict);
+void hmp_test_vmstates_set_period(Monitor *mon, const QDict *qdict);
 void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
 void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
 void device_add_completion(ReadLineState *rs, int nb_args, const char *str);
-- 
1.9.3

[Qemu-devel] [PATCH RFC v2 11/12] VMState test: cancel mechanism for an already running vmstate testing process

[Sanidhya Kashyap]

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 qapi-schema.json |  9 +++++++++
 qmp-commands.hx  | 19 +++++++++++++++++++
 savevm.c         | 16 ++++++++++++++--
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 13e922e..91f1672 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3574,3 +3574,12 @@
 ##
 { 'command' : 'test-vmstates-set-period',
   'data'    : { 'period': 'int' } }
+
+##
+# @log-dirty-bitmap-cancel
+#
+# cancel the testing vmstates process
+#
+# Since 2.2
+##
+{ 'command': 'test-vmstates-cancel' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2f019b0..1035885 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3865,3 +3865,22 @@ Example:
 <- { "return": {} }
 EQMP
 
+	{
+        .name       = "test-vmstates-cancel",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_test_vmstates_cancel,
+    },
+
+SQMP
+test-vmstates-cancel
+--------------
+
+Cancel the current vmstate testing process.
+
+Arguments: None.
+
+Example:
+
+-> { "execute": "test-vmstates-cancel" }
+<- { "return": {} }
+EQMP
diff --git a/savevm.c b/savevm.c
index 8b75691..66597db 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1365,8 +1365,12 @@ static void vmstate_test_cb(void *opaque)
         if (saved_vm_running) {
             vm_start();
         }
-        timer_mod(v->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
-                                              v->period);
+       if (v->active_state) {
+            timer_mod(v->timer, v->period +
+                      qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
+        } else {
+            goto testing_end;
+        }
         return;
     }
 
@@ -1482,6 +1486,14 @@ void qmp_test_vmstates_set_period(int64_t period, Error **errp)
     v->period = period;
 }
 
+void qmp_test_vmstates_cancel(Error **errp)
+{
+    VMStateLogState *v = vmstate_current_state();
+    if (v->active_state) {
+        v->active_state = false;
+    }
+}
+
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC v2 11/12] VMState test: cancel mechanism for an already running vmstate testing process

[Eric Blake]

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

On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---
>  qapi-schema.json |  9 +++++++++
>  qmp-commands.hx  | 19 +++++++++++++++++++
>  savevm.c         | 16 ++++++++++++++--
>  3 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 13e922e..91f1672 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3574,3 +3574,12 @@
>  ##
>  { 'command' : 'test-vmstates-set-period',
>    'data'    : { 'period': 'int' } }
> +
> +##
> +# @log-dirty-bitmap-cancel
> +#
> +# cancel the testing vmstates process
> +#
> +# Since 2.2
> +##
> +{ 'command': 'test-vmstates-cancel' }

Command name doesn't match documentation.


> +
> +SQMP
> +test-vmstates-cancel
> +--------------

Make the ---- line up to the command name.


> +++ b/savevm.c
> @@ -1365,8 +1365,12 @@ static void vmstate_test_cb(void *opaque)
>          if (saved_vm_running) {
>              vm_start();
>          }
> -        timer_mod(v->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> -                                              v->period);
> +       if (v->active_state) {

Indentation is off.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH RFC v2 12/12] VMState test: hmp interface for cancel mechanism

[Sanidhya Kashyap]

Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
---
 hmp-commands.hx | 14 ++++++++++++++
 hmp.c           |  6 ++++++
 hmp.h           |  1 +
 3 files changed, 21 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 6d15184..fe224fc 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1822,6 +1822,20 @@ STEXI
 Set the period to @var{period} (int) for vmstate testing process.
 ETEXI
 
+	{
+	.name       = "test_vmstates_cancel",
+	.args_type  = "",
+	.params     = "",
+	.help       = "cancel the current vmstates testing process",
+	.mhandler.cmd = hmp_test_vmstates_cancel,
+},
+
+STEXI
+@item test_vmstates_cancel
+@findex test_vmstates_cancel
+Cancel the current vmstates testing process
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index f54b0b9..bbff92a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1781,3 +1781,9 @@ void hmp_test_vmstates_set_period(Monitor *mon, const QDict *qdict)
         error_free(err);
     }
 }
+
+void hmp_test_vmstates_cancel(Monitor *mon, const QDict *qdict)
+{
+   qmp_test_vmstates_cancel(NULL);
+}
+
diff --git a/hmp.h b/hmp.h
index e1afde8..1277dbc 100644
--- a/hmp.h
+++ b/hmp.h
@@ -98,6 +98,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict);
 void hmp_info_memdev(Monitor *mon, const QDict *qdict);
 void hmp_test_vmstates(Monitor *mon, const QDict *qdict);
 void hmp_test_vmstates_set_period(Monitor *mon, const QDict *qdict);
+void hmp_test_vmstates_cancel(Monitor *mon, const QDict *qdict);
 void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
 void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
 void device_add_completion(ReadLineState *rs, int nb_args, const char *str);
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC v2 12/12] VMState test: hmp interface for cancel mechanism

[Eric Blake]

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

On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---
>  hmp-commands.hx | 14 ++++++++++++++
>  hmp.c           |  6 ++++++
>  hmp.h           |  1 +
>  3 files changed, 21 insertions(+)

I don't mind if you squash the QMP and HMP counterpart commands into the
same patch as one another (throughout the series).  Each pair of patches
are both small enough, and related to the same action, that doing it as
a combined patch may actually get a better review.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH RFC v2 12/12] VMState test: hmp interface for cancel mechanism

[Sanidhya Kashyap]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 07/29/2014 10:22 PM, Eric Blake wrote:
> On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
>> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com> --- 
>> hmp-commands.hx | 14 ++++++++++++++ hmp.c           |  6 ++++++ 
>> hmp.h           |  1 + 3 files changed, 21 insertions(+)
> 
> I don't mind if you squash the QMP and HMP counterpart commands
> into the same patch as one another (throughout the series).  Each
> pair of patches are both small enough, and related to the same
> action, that doing it as a combined patch may actually get a better
> review.
> 

Will keep this in mind. Thanks!

- -- 
- -----

Sanidhya Kashyap
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJT1+KeAAoJEFt9RLmoahlnrCgIAJanxIlvyKjoTTzRobMrwg8b
qIVSqS1uT0Fn/vt32faIjfwyOhb4E/mcHamT/wau/zLRvD+vQh7kyRYWfqL4YI/0
N2txLlTqI867fF6CzZPx3QD210N8bNMofnWcpSP4TXhrfj6O/AcN3B0Dyja7cA2R
q0YXRMPf3t8jSKYKwHq0IafC9bLS+pcgTG1YN4qN9LIeEilP4ZBRxDeAxDkSUuPx
0+pxGxd83fWYLziF4phI7qFurufjeOT1oKdx67jahd+Flpc3WZetTGoZw5iB8yb/
u9WpjoFLJ13AB/BiyLP+BrL89muxnZefpE5Id1Vjiz635deAbyFdzypuTUKVY34=
=QgfL
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH RFC v2 12/12] VMState test: hmp interface for cancel mechanism

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/25/2014 09:39 AM, Sanidhya Kashyap wrote:
>> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
>> ---
>>  hmp-commands.hx | 14 ++++++++++++++
>>  hmp.c           |  6 ++++++
>>  hmp.h           |  1 +
>>  3 files changed, 21 insertions(+)
>
> I don't mind if you squash the QMP and HMP counterpart commands into the
> same patch as one another (throughout the series).  Each pair of patches
> are both small enough, and related to the same action, that doing it as
> a combined patch may actually get a better review.

It's okay to squash them when they're small and the interfaces they add
are uncontroversial.  Otherwise, I prefer them separate, so I can
concentrate on the QMP interface first, without HMP distractions.