Skip to contentSkip to conversation

Bring savevm.c to the wonderful world of sys-queue.h

PATCH In review Juan Quintela 3 participants 9 messages Aug 28, 2009 – Sep 1, 2009

lore
Share & tools
lore
#0 lore raw

cover Bring savevm.c to the wonderful world of sys-queue.h

This series:
- replace first_se by a TAILQ (savevm_handlers)
- replace first_le by a LIST (loadvm_handlers)
- add LIST_FOREACH_SAFe that didn't exist before

Later, Juan.

Juan Quintela (3):
savevm: Convert savevm handlers list to TAILQ
Add LIST_FOREACH_SAFE() definition
savevm: Convert loadvm handlers list to LIST

 savevm.c    |  114 +++++++++++++++++++++++++++-------------------------------
 sys-queue.h |    5 +++
 2 files changed, 58 insertions(+), 61 deletions(-)
#1 lore raw

PATCH 1/3 savevm: Convert savevm handlers list to TAILQ

---
 savevm.c |   94 +++++++++++++++++++++++++++----------------------------------
 1 files changed, 42 insertions(+), 52 deletions(-)
savevm.c +42 -52
index 2b4054a..baef277 100644
@@ -890,6 +890,7 @@ const VMStateInfo vmstate_info_buffer = {
 };
 
 typedef struct SaveStateEntry {
+    TAILQ_ENTRY(SaveStateEntry) entry;
     char idstr[256];
     int instance_id;
     int version_id;
@@ -899,12 +900,26 @@ typedef struct SaveStateEntry {
     LoadStateHandler *load_state;
     const VMStateDescription *vmsd;
     void *opaque;
-    struct SaveStateEntry *next;
 } SaveStateEntry;
 
-static SaveStateEntry *first_se;
+static TAILQ_HEAD(savevm_handlers, SaveStateEntry) savevm_handlers =
+    TAILQ_HEAD_INITIALIZER(savevm_handlers);
 static int global_section_id;
 
+static int calculate_new_instance_id(const char *idstr)
+{
+    SaveStateEntry *se;
+    int instance_id = 0;
+
+    TAILQ_FOREACH(se, &savevm_handlers, entry) {
+        if (strcmp(idstr, se->idstr) == 0
+            && instance_id <= se->instance_id) {
+            instance_id = se->instance_id + 1;
+        }
+    }
+    return instance_id;
+}
+
 /* TODO: Individual devices generally have very little idea about the rest
    of the system, so instance_id should be removed/replaced.
    Meanwhile pass -1 as instance_id if you do not already have a clearly
@@ -917,11 +932,10 @@ int register_savevm_live(const char *idstr,
                          LoadStateHandler *load_state,
                          void *opaque)
 {
-    SaveStateEntry *se, **pse;
+    SaveStateEntry *se;
 
     se = qemu_malloc(sizeof(SaveStateEntry));
     pstrcpy(se->idstr, sizeof(se->idstr), idstr);
-    se->instance_id = (instance_id == -1) ? 0 : instance_id;
     se->version_id = version_id;
     se->section_id = global_section_id++;
     se->save_live_state = save_live_state;
@@ -929,18 +943,14 @@ int register_savevm_live(const char *idstr,
     se->load_state = load_state;
     se->opaque = opaque;
     se->vmsd = NULL;
-    se->next = NULL;
 
-    /* add at the end of list */
-    pse = &first_se;
-    while (*pse != NULL) {
-        if (instance_id == -1
-                && strcmp(se->idstr, (*pse)->idstr) == 0
-                && se->instance_id <= (*pse)->instance_id)
-            se->instance_id = (*pse)->instance_id + 1;
-        pse = &(*pse)->next;
+    if (instance_id == -1) {
+        se->instance_id = calculate_new_instance_id(idstr);
+    } else {
+        se->instance_id = instance_id;
     }
-    *pse = se;
+    /* add at the end of list */
+    TAILQ_INSERT_TAIL(&savevm_handlers, se, entry);
     return 0;
 }
 
@@ -957,28 +967,23 @@ int register_savevm(const char *idstr,
 
 void unregister_savevm(const char *idstr, void *opaque)
 {
-    SaveStateEntry **pse;
+    SaveStateEntry *se, *new_se;
 
-    pse = &first_se;
-    while (*pse != NULL) {
-        if (strcmp((*pse)->idstr, idstr) == 0 && (*pse)->opaque == opaque) {
-            SaveStateEntry *next = (*pse)->next;
-            qemu_free(*pse);
-            *pse = next;
-            continue;
+    TAILQ_FOREACH_SAFE(se, &savevm_handlers, entry, new_se) {
+        if (strcmp(se->idstr, idstr) == 0 && se->opaque == opaque) {
+            TAILQ_REMOVE(&savevm_handlers, se, entry);
+            qemu_free(se);
         }
-        pse = &(*pse)->next;
     }
 }
 
 int vmstate_register(int instance_id, const VMStateDescription *vmsd,
                      void *opaque)
 {
-    SaveStateEntry *se, **pse;
+    SaveStateEntry *se;
 
     se = qemu_malloc(sizeof(SaveStateEntry));
     pstrcpy(se->idstr, sizeof(se->idstr), vmsd->name);
-    se->instance_id = (instance_id == -1) ? 0 : instance_id;
     se->version_id = vmsd->version_id;
     se->section_id = global_section_id++;
     se->save_live_state = NULL;
@@ -986,35 +991,20 @@ int vmstate_register(int instance_id, const VMStateDescription *vmsd,
     se->load_state = NULL;
     se->opaque = opaque;
     se->vmsd = vmsd;
-    se->next = NULL;
 
-    /* add at the end of list */
-    pse = &first_se;
-    while (*pse != NULL) {
-        if (instance_id == -1
-                && strcmp(se->idstr, (*pse)->idstr) == 0
-                && se->instance_id <= (*pse)->instance_id)
-            se->instance_id = (*pse)->instance_id + 1;
-        pse = &(*pse)->next;
+    if (instance_id == -1) {
+        se->instance_id = calculate_new_instance_id(vmsd->name);
+    } else {
+        se->instance_id = instance_id;
     }
-    *pse = se;
+    /* add at the end of list */
+    TAILQ_INSERT_TAIL(&savevm_handlers, se, entry);
     return 0;
 }
 
 void vmstate_unregister(const char *idstr,  void *opaque)
 {
-    SaveStateEntry **pse;
-
-    pse = &first_se;
-    while (*pse != NULL) {
-        if (strcmp((*pse)->idstr, idstr) == 0 && (*pse)->opaque == opaque) {
-            SaveStateEntry *next = (*pse)->next;
-            qemu_free(*pse);
-            *pse = next;
-            continue;
-        }
-        pse = &(*pse)->next;
-    }
+    unregister_savevm(idstr, opaque);
 }
 
 int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
@@ -1129,7 +1119,7 @@ int qemu_savevm_state_begin(QEMUFile *f)
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
 
-    for (se = first_se; se != NULL; se = se->next) {
+    TAILQ_FOREACH(se, &savevm_handlers, entry) {
         int len;
 
         if (se->save_live_state == NULL)
@@ -1161,7 +1151,7 @@ int qemu_savevm_state_iterate(QEMUFile *f)
     SaveStateEntry *se;
     int ret = 1;
 
-    for (se = first_se; se != NULL; se = se->next) {
+    TAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (se->save_live_state == NULL)
             continue;
 
@@ -1185,7 +1175,7 @@ int qemu_savevm_state_complete(QEMUFile *f)
 {
     SaveStateEntry *se;
 
-    for (se = first_se; se != NULL; se = se->next) {
+    TAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (se->save_live_state == NULL)
             continue;
 
@@ -1196,7 +1186,7 @@ int qemu_savevm_state_complete(QEMUFile *f)
         se->save_live_state(f, QEMU_VM_SECTION_END, se->opaque);
     }
 
-    for(se = first_se; se != NULL; se = se->next) {
+    TAILQ_FOREACH(se, &savevm_handlers, entry) {
         int len;
 
 	if (se->save_state == NULL && se->vmsd == NULL)
@@ -1261,7 +1251,7 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id)
 {
     SaveStateEntry *se;
 
-    for(se = first_se; se != NULL; se = se->next) {
+    TAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (!strcmp(se->idstr, idstr) &&
             instance_id == se->instance_id)
             return se;
Signature · 1 line
1.6.2.5
#2 lore raw

Re: [Qemu-devel] [PATCH 1/3] savevm: Convert savevm handlers list to TAILQ

Hi,

Quoted 5 lines

int vmstate_register(int instance_id, const VMStateDescription *vmsd,
void *opaque)

  void vmstate_unregister(const char *idstr,  void *opaque)
  {

vmstate_unregister() should accept the same parameters passed to vmstate_register(). idstr does not fit here. Should probably be either vmsd + opaque or just opaque.

cheers,
Gerd

#3 lore raw

PATCH 2/3 Add LIST_FOREACH_SAFE() definition

In reply to Juan Quintela
---
 sys-queue.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
sys-queue.h +5 -0
index cb6a4c8..eb89a4d 100644
@@ -126,6 +126,11 @@ struct {                                                                \
                 (var);                                                  \
                 (var) = ((var)->field.le_next))
 
+#define LIST_FOREACH_SAFE(var, head, field, next_var)                   \
+        for ((var) = ((head)->lh_first);                                \
+                (var) && ((next_var) = ((var)->field.le_next), 1);      \
+                (var) = (next_var))
+
 /*
  * List access methods.
  */
Signature · 1 line
1.6.2.5
#4 lore raw

PATCH 3/3 savevm: Convert loadvm handlers list to LIST

In reply to Juan Quintela
---
 savevm.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)
savevm.c +11 -9
index baef277..9836c60 100644
@@ -1260,10 +1260,10 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id)
 }
 
 typedef struct LoadStateEntry {
+    LIST_ENTRY(LoadStateEntry) entry;
     SaveStateEntry *se;
     int section_id;
     int version_id;
-    struct LoadStateEntry *next;
 } LoadStateEntry;
 
 static int qemu_loadvm_state_v2(QEMUFile *f)
@@ -1309,7 +1309,8 @@ static int qemu_loadvm_state_v2(QEMUFile *f)
 
 int qemu_loadvm_state(QEMUFile *f)
 {
-    LoadStateEntry *first_le = NULL;
+    LIST_HEAD(, LoadStateEntry) loadvm_handlers;
+    LoadStateEntry *le, *new_le;
     uint8_t section_type;
     unsigned int v;
     int ret;
@@ -1326,7 +1327,6 @@ int qemu_loadvm_state(QEMUFile *f)
 
     while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
         uint32_t instance_id, version_id, section_id;
-        LoadStateEntry *le;
         SaveStateEntry *se;
         char idstr[257];
         int len;
@@ -1364,8 +1364,7 @@ int qemu_loadvm_state(QEMUFile *f)
             le->se = se;
             le->section_id = section_id;
             le->version_id = version_id;
-            le->next = first_le;
-            first_le = le;
+            LIST_INSERT_HEAD(&loadvm_handlers, le, entry);
 
             ret = vmstate_load(f, le->se, le->version_id);
             if (ret < 0) {
@@ -1378,7 +1377,11 @@ int qemu_loadvm_state(QEMUFile *f)
         case QEMU_VM_SECTION_END:
             section_id = qemu_get_be32(f);
 
-            for (le = first_le; le && le->section_id != section_id; le = le->next);
+            LIST_FOREACH(le, &loadvm_handlers, entry) {
+                if (le->section_id == section_id) {
+                    break;
+                }
+            }
             if (le == NULL) {
                 fprintf(stderr, "Unknown savevm section %d\n", section_id);
                 ret = -EINVAL;
@@ -1402,9 +1405,8 @@ int qemu_loadvm_state(QEMUFile *f)
     ret = 0;
 
 out:
-    while (first_le) {
-        LoadStateEntry *le = first_le;
-        first_le = first_le->next;
+    LIST_FOREACH_SAFE(le, &loadvm_handlers, entry, new_le) {
+        LIST_REMOVE(le, entry);
         qemu_free(le);
     }
 
Signature · 1 line
1.6.2.5
#5 lore raw

Re: [Qemu-devel] [PATCH 3/3] savevm: Convert loadvm handlers list to LIST

Quoted 23 lines · On Fri, 28 Aug 2009 22:31:57 +0200 Juan Quintela <[email protected]> wrote:
---
 savevm.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)
savevm.c +1 -1
index baef277..9836c60 100644
@@ -1260,10 +1260,10 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id)
 }
 
 typedef struct LoadStateEntry {
+    LIST_ENTRY(LoadStateEntry) entry;
     SaveStateEntry *se;
     int section_id;
     int version_id;
-    struct LoadStateEntry *next;
 } LoadStateEntry;
 
 static int qemu_loadvm_state_v2(QEMUFile *f)

savevm.c static int qemu_loadvm_state_v2(QEMUFile *f) · line 1312

Quoted 5 lines
savevm.c +1 -1
@@ -1309,7 +1309,8 @@ static int qemu_loadvm_state_v2(QEMUFile *f)
 
 int qemu_loadvm_state(QEMUFile *f)
 {
-    LoadStateEntry *first_le = NULL;
+    LIST_HEAD(, LoadStateEntry) loadvm_handlers;

You're missing the initialization here, spot this while
testing staging.

#6 lore raw

[Qemu-devel] Re: [PATCH 3/3] savevm: Convert loadvm handlers list to LIST

Quoted 27 lines · Luiz Capitulino
Quoted 24 lines · On Fri, 28 Aug 2009 22:31:57 +0200 Juan Quintela <[email protected]> wrote:
---
 savevm.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)
savevm.c +1 -1
index baef277..9836c60 100644
@@ -1260,10 +1260,10 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id)
 }
 
 typedef struct LoadStateEntry {
+    LIST_ENTRY(LoadStateEntry) entry;
     SaveStateEntry *se;
     int section_id;
     int version_id;
-    struct LoadStateEntry *next;
 } LoadStateEntry;
 
 static int qemu_loadvm_state_v2(QEMUFile *f)
@@ -1309,7 +1309,8 @@ static int qemu_loadvm_state_v2(QEMUFile *f)
 
Quoted 7 lines

savevm.c static int qemu_loadvm_state_v2(QEMUFile *f) · line 1312

Quoted 4 lines
savevm.c +1 -1
 int qemu_loadvm_state(QEMUFile *f)
 {
-    LoadStateEntry *first_le = NULL;
+    LIST_HEAD(, LoadStateEntry) loadvm_handlers;

You're missing the initialization here, spot this while
testing staging.

I looked at aio.c and guess what :) No LIST_INIT() either. My understanding is that it is not needed (but it is better to add it, just in case the implementation change).

#define LIST_HEAD_INITIALIZER(head)                                     \
        { NULL }
#define LIST_INIT(head) do {                                            \
        (head)->lh_first = NULL;                                        \
} while (/*CONSTCOND*/0)

This should be what it does without puting it. Perhaps we should "correct" also the other users?

Later, Juan.

#7 lore raw

[Qemu-devel] Re: [PATCH 3/3] savevm: Convert loadvm handlers list to LIST

Quoted 36 lines · On Tue, 01 Sep 2009 00:08:43 +0200 Juan Quintela <[email protected]> wrote:
Quoted 27 lines · Luiz Capitulino
Quoted 24 lines · On Fri, 28 Aug 2009 22:31:57 +0200 Juan Quintela <[email protected]> wrote:
---
 savevm.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)
savevm.c +1 -1
index baef277..9836c60 100644
@@ -1260,10 +1260,10 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id)
 }
 
 typedef struct LoadStateEntry {
+    LIST_ENTRY(LoadStateEntry) entry;
     SaveStateEntry *se;
     int section_id;
     int version_id;
-    struct LoadStateEntry *next;
 } LoadStateEntry;
 
 static int qemu_loadvm_state_v2(QEMUFile *f)
@@ -1309,7 +1309,8 @@ static int qemu_loadvm_state_v2(QEMUFile *f)
 
Quoted 7 lines

savevm.c static int qemu_loadvm_state_v2(QEMUFile *f) · line 1312

Quoted 4 lines
savevm.c +1 -1
 int qemu_loadvm_state(QEMUFile *f)
 {
-    LoadStateEntry *first_le = NULL;
+    LIST_HEAD(, LoadStateEntry) loadvm_handlers;

You're missing the initialization here, spot this while
testing staging.

Quoted 1 line

I looked at aio.c and guess what :) No LIST_INIT() either.

As we talked by irc, if you are referring to 'aio_handlers' it's
global, so it will be initialized to 0 by the kernel.

Quoted 2 lines

My understanding is that it is not needed (but it is better to add it, just in case the implementation change).

I'm getting a segfault when I try to loadvm, I can write a recipe
to reproduce if needed.

Quoted 10 lines
#define LIST_HEAD_INITIALIZER(head)                                     \
        { NULL }
#define LIST_INIT(head) do {                                            \
        (head)->lh_first = NULL;                                        \
} while (/*CONSTCOND*/0)

This should be what it does without puting it. Perhaps we should "correct" also the other users?

Yes, IMHO. Not because it's a fix, but it's good practice to use the API
w/o making assumptions on its internals.

#8 lore raw

[Qemu-devel] Re: [PATCH 3/3] savevm: Convert loadvm handlers list to LIST

Quoted 39 lines · Luiz Capitulino
Quoted 36 lines · On Tue, 01 Sep 2009 00:08:43 +0200 Juan Quintela <[email protected]> wrote:
Quoted 34 lines · Luiz Capitulino
Quoted 23 lines · On Fri, 28 Aug 2009 22:31:57 +0200 Juan Quintela <[email protected]> wrote:
---
 savevm.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)
savevm.c +1 -1
index baef277..9836c60 100644
@@ -1260,10 +1260,10 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id)
 }
 
 typedef struct LoadStateEntry {
+    LIST_ENTRY(LoadStateEntry) entry;
     SaveStateEntry *se;
     int section_id;
     int version_id;
-    struct LoadStateEntry *next;
 } LoadStateEntry;
 
 static int qemu_loadvm_state_v2(QEMUFile *f)

savevm.c static int qemu_loadvm_state_v2(QEMUFile *f) · line 1312

Quoted 5 lines
savevm.c +1 -1
@@ -1309,7 +1309,8 @@ static int qemu_loadvm_state_v2(QEMUFile *f)
 
 int qemu_loadvm_state(QEMUFile *f)
 {
-    LoadStateEntry *first_le = NULL;
+    LIST_HEAD(, LoadStateEntry) loadvm_handlers;

You're missing the initialization here, spot this while
testing staging.

Quoted 4 lines
Quoted 1 line

I looked at aio.c and guess what :) No LIST_INIT() either.

As we talked by irc, if you are referring to 'aio_handlers' it's
global, so it will be initialized to 0 by the kernel.

My bad. In this case, I am just a lucky man, and in all my testing, I always got the list intialized correctly :)

Patches fixing it sent already.

Thanks a lot, Juan.

Quoted 19 lines
Quoted 2 lines

My understanding is that it is not needed (but it is better to add it, just in case the implementation change).

I'm getting a segfault when I try to loadvm, I can write a recipe
to reproduce if needed.

Quoted 10 lines
#define LIST_HEAD_INITIALIZER(head)                                     \
        { NULL }
#define LIST_INIT(head) do {                                            \
        (head)->lh_first = NULL;                                        \
} while (/*CONSTCOND*/0)

This should be what it does without puting it. Perhaps we should "correct" also the other users?

Yes, IMHO. Not because it's a fix, but it's good practice to use the API
w/o making assumptions on its internals.