[Qemu-devel] Re: [PATCH 15/19] savevm: introduce qemu_savevm_trans_{begin, commit}.

2011-02-02 Thread Yoshiaki Tamura 
2011/2/2 Paolo Bonzini pbonz...@redhat.com:
 On 02/01/2011 07:21 PM, Yoshiaki Tamura wrote:

 Paolo,

 I refactored the savevm functions.  Could you give me your
 comments?

 I didn't review it thoroughly, but the abstractions seem okay.

Thanks.  Since It got a bit messy, I wanted hear your opinion.

Yoshi


 Paolo
 --
 To unsubscribe from this list: send the line unsubscribe kvm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Qemu-devel] Re: [PATCH 15/19] savevm: introduce qemu_savevm_trans_{begin, commit}.

2011-02-02 Thread Paolo Bonzini 

On 02/01/2011 07:21 PM, Yoshiaki Tamura wrote:

Paolo,

I refactored the savevm functions.  Could you give me your
comments?


I didn't review it thoroughly, but the abstractions seem okay.

Paolo

[Qemu-devel] Re: [PATCH 15/19] savevm: introduce qemu_savevm_trans_{begin, commit}.

2011-02-01 Thread Yoshiaki Tamura 
Paolo,

I refactored the savevm functions.  Could you give me your
comments?

Thanks,

Yoshi

diff --git a/savevm.c b/savevm.c
index 5418280..90aae55 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1602,29 +1602,68 @@ bool qemu_savevm_state_blocked(Monitor *mon)
 return false;
 }

-int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
-int shared)
+/*
+ * section: header to write
+ * inc: if true, forces to pass SECTION_PART instead of SECTION_START
+ * pause: if true, breaks the loop when live handler returned 0
+ */
+static int qemu_savevm_state_live(Monitor *mon, QEMUFile *f, int section,
+  bool inc, bool pause)
 {
 SaveStateEntry *se;
+int skip = 0, ret;

 QTAILQ_FOREACH(se, savevm_handlers, entry) {
-if(se-set_params == NULL) {
+int len, stage;
+
+if (se-save_live_state == NULL) {
 continue;
-   }
-   se-set_params(blk_enable, shared, se-opaque);
+}
+
+/* Section type */
+qemu_put_byte(f, section);
+qemu_put_be32(f, se-section_id);
+
+if (section == QEMU_VM_SECTION_START) {
+/* ID string */
+len = strlen(se-idstr);
+qemu_put_byte(f, len);
+qemu_put_buffer(f, (uint8_t *)se-idstr, len);
+
+qemu_put_be32(f, se-instance_id);
+qemu_put_be32(f, se-version_id);
+
+stage = inc ? QEMU_VM_SECTION_PART : QEMU_VM_SECTION_START;
+} else {
+assert(inc);
+stage = section;
+}
+
+ret = se-save_live_state(mon, f, stage, se-opaque);
+if (!ret) {
+skip++;
+if (pause) {
+break;
+}
+}
 }
-
-qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
-qemu_put_be32(f, QEMU_VM_FILE_VERSION);
+
+return skip;
+}
+
+static void qemu_savevm_state_full(QEMUFile *f)
+{
+SaveStateEntry *se;

 QTAILQ_FOREACH(se, savevm_handlers, entry) {
 int len;

-if (se-save_live_state == NULL)
+if (se-save_state == NULL  se-vmsd == NULL) {
 continue;
+}

 /* Section type */
-qemu_put_byte(f, QEMU_VM_SECTION_START);
+qemu_put_byte(f, QEMU_VM_SECTION_FULL);
 qemu_put_be32(f, se-section_id);

 /* ID string */
@@ -1635,8 +1674,28 @@ int qemu_savevm_state_begin(Monitor *mon,
QEMUFile *f, int blk_enable,
 qemu_put_be32(f, se-instance_id);
 qemu_put_be32(f, se-version_id);

-se-save_live_state(mon, f, QEMU_VM_SECTION_START, se-opaque);
+vmstate_save(f, se);
+}
+
+qemu_put_byte(f, QEMU_VM_EOF);
+}
+
+int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
+int shared)
+{
+SaveStateEntry *se;
+
+QTAILQ_FOREACH(se, savevm_handlers, entry) {
+if(se-set_params == NULL) {
+continue;
+}
+se-set_params(blk_enable, shared, se-opaque);
 }
+
+qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
+qemu_put_be32(f, QEMU_VM_FILE_VERSION);
+
+qemu_savevm_state_live(mon, f, QEMU_VM_SECTION_START, 0, 0);

 if (qemu_file_has_error(f)) {
 qemu_savevm_state_cancel(mon, f);
@@ -1648,29 +1707,16 @@ int qemu_savevm_state_begin(Monitor *mon,
QEMUFile *f, int blk_enable,

 int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
 {
-SaveStateEntry *se;
 int ret = 1;

-QTAILQ_FOREACH(se, savevm_handlers, entry) {
-if (se-save_live_state == NULL)
-continue;
-
-/* Section type */
-qemu_put_byte(f, QEMU_VM_SECTION_PART);
-qemu_put_be32(f, se-section_id);
-
-ret = se-save_live_state(mon, f, QEMU_VM_SECTION_PART, se-opaque);
-if (!ret) {
-/* Do not proceed to the next vmstate before this one reported
-   completion of the current stage. This serializes the migration
-   and reduces the probability that a faster changing state is
-   synchronized over and over again. */
-break;
-}
-}
-
-if (ret)
+/* Do not proceed to the next vmstate before this one reported
+   completion of the current stage. This serializes the migration
+   and reduces the probability that a faster changing state is
+   synchronized over and over again. */
+ret = qemu_savevm_state_live(mon, f, QEMU_VM_SECTION_PART, 1, 1);
+if (!ret) {
 return 1;
+}

 if (qemu_file_has_error(f)) {
 qemu_savevm_state_cancel(mon, f);
@@ -1682,46 +1728,40 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)

 int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
 {
-SaveStateEntry *se;
-
 cpu_synchronize_all_states();

-QTAILQ_FOREACH(se, savevm_handlers, entry) {
-if (se-save_live_state == NULL)
-continue;
-
-/* Section type */
-qemu_put_byte(f, QEMU_VM_SECTION_END);
-qemu_put_be32(f,

[Qemu-devel] Re: [PATCH 15/19] savevm: introduce qemu_savevm_trans_{begin, commit}.

2011-01-28 Thread Paolo Bonzini 

On 01/28/2011 08:21 AM, Yoshiaki Tamura wrote:

+int qemu_savevm_trans_begin(Monitor *mon, QEMUFile *f, int init)
+{
+SaveStateEntry *se;
+int skipped = 0;
+
+QTAILQ_FOREACH(se,savevm_handlers, entry) {
+int len, stage, ret;
+
+if (se-save_live_state == NULL) {
+continue;
+}
+
+/* Section type */
+qemu_put_byte(f, QEMU_VM_SECTION_START);
+qemu_put_be32(f, se-section_id);
+
+/* ID string */
+len = strlen(se-idstr);
+qemu_put_byte(f, len);
+qemu_put_buffer(f, (uint8_t *)se-idstr, len);
+
+qemu_put_be32(f, se-instance_id);
+qemu_put_be32(f, se-version_id);
+
+stage = init ? QEMU_VM_SECTION_START : QEMU_VM_SECTION_PART;
+ret = se-save_live_state(mon, f, stage, se-opaque);
+if (!ret) {
+skipped++;
+}
+}
+
+if (qemu_file_has_error(f)) {
+return -EIO;
+}
+
+return skipped;
+}
+


Right now, this is very similar to qemu_savevm_state_begin and _iterate, 
but not quite.  Perhaps you could abstract it to a single function that 
could be used everywhere live handlers are used.  For example,


/* section says which header to write; incremental == true forces to
   pass SECTION_PART instead of SECTION_START. In code:

if (section == QEMU_VM_SECTION_START) {
stage = incremental ? QEMU_VM_SECTION_PART : QEMU_VM_SECTION_START;
} else {
assert(incremental);
stage = section;
}

   */
int qemu_savevm_state_live(Monitor *mon, QEMUFile *f, int section,
   int incremental)

Likewise,


+QTAILQ_FOREACH(se, savevm_handlers, entry) {
+int len;
+
+if (se-save_state == NULL  se-vmsd == NULL) {
+continue;
+}
+
+/* Section type */
+qemu_put_byte(f, QEMU_VM_SECTION_FULL);
+qemu_put_be32(f, se-section_id);
+
+/* ID string */
+len = strlen(se-idstr);
+qemu_put_byte(f, len);
+qemu_put_buffer(f, (uint8_t *)se-idstr, len);
+
+qemu_put_be32(f, se-instance_id);
+qemu_put_be32(f, se-version_id);
+
+vmstate_save(f, se);
+}


this code is straight from qemu_savevm_state_complete and should be 
moved into its own function.


Paolo

[Qemu-devel] Re: [PATCH 15/19] savevm: introduce qemu_savevm_trans_{begin, commit}.

2011-01-28 Thread Yoshiaki Tamura 
2011/1/28 Paolo Bonzini pbonz...@redhat.com:
 On 01/28/2011 08:21 AM, Yoshiaki Tamura wrote:

 +int qemu_savevm_trans_begin(Monitor *mon, QEMUFile *f, int init)
 +{
 +    SaveStateEntry *se;
 +    int skipped = 0;
 +
 +    QTAILQ_FOREACH(se,savevm_handlers, entry) {
 +        int len, stage, ret;
 +
 +        if (se-save_live_state == NULL) {
 +            continue;
 +        }
 +
 +        /* Section type */
 +        qemu_put_byte(f, QEMU_VM_SECTION_START);
 +        qemu_put_be32(f, se-section_id);
 +
 +        /* ID string */
 +        len = strlen(se-idstr);
 +        qemu_put_byte(f, len);
 +        qemu_put_buffer(f, (uint8_t *)se-idstr, len);
 +
 +        qemu_put_be32(f, se-instance_id);
 +        qemu_put_be32(f, se-version_id);
 +
 +        stage = init ? QEMU_VM_SECTION_START : QEMU_VM_SECTION_PART;
 +        ret = se-save_live_state(mon, f, stage, se-opaque);
 +        if (!ret) {
 +            skipped++;
 +        }
 +    }
 +
 +    if (qemu_file_has_error(f)) {
 +        return -EIO;
 +    }
 +
 +    return skipped;
 +}
 +

 Right now, this is very similar to qemu_savevm_state_begin and _iterate, but
 not quite.  Perhaps you could abstract it to a single function that could be
 used everywhere live handlers are used.  For example,

 /* section says which header to write; incremental == true forces to
   pass SECTION_PART instead of SECTION_START. In code:

 if (section == QEMU_VM_SECTION_START) {
    stage = incremental ? QEMU_VM_SECTION_PART : QEMU_VM_SECTION_START;
 } else {
    assert(incremental);
    stage = section;
 }

   */
 int qemu_savevm_state_live(Monitor *mon, QEMUFile *f, int section,
                           int incremental)

 Likewise,

 +    QTAILQ_FOREACH(se, savevm_handlers, entry) {
 +        int len;
 +
 +        if (se-save_state == NULL  se-vmsd == NULL) {
 +            continue;
 +        }
 +
 +        /* Section type */
 +        qemu_put_byte(f, QEMU_VM_SECTION_FULL);
 +        qemu_put_be32(f, se-section_id);
 +
 +        /* ID string */
 +        len = strlen(se-idstr);
 +        qemu_put_byte(f, len);
 +        qemu_put_buffer(f, (uint8_t *)se-idstr, len);
 +
 +        qemu_put_be32(f, se-instance_id);
 +        qemu_put_be32(f, se-version_id);
 +
 +        vmstate_save(f, se);
 +    }

 this code is straight from qemu_savevm_state_complete and should be moved
 into its own function.

Looks reasonable to avoid bit rotten.  Let me see what I can do.

Thanks,

Yoshi


 Paolo

 --
 To unsubscribe from this list: send the line unsubscribe kvm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html