[Qemu-devel] [PATCH 0/3] Bring savevm.c to the wonderful world of sys-queue.h

[Qemu-devel] [PATCH 0/3] Bring savevm.c to the wonderful world of sys-queue.h

[Juan Quintela]

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(-)

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

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |   94 +++++++++++++++++++++++++++----------------------------------
 1 files changed, 42 insertions(+), 52 deletions(-)

diff --git a/savevm.c b/savevm.c
index 2b4054a..baef277 100644
--- a/savevm.c
+++ b/savevm.c
@@ -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;
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/3] Add LIST_FOREACH_SAFE() definition

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 sys-queue.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/sys-queue.h b/sys-queue.h
index cb6a4c8..eb89a4d 100644
--- a/sys-queue.h
+++ b/sys-queue.h
@@ -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.
  */
-- 
1.6.2.5

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

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/savevm.c b/savevm.c
index baef277..9836c60 100644
--- a/savevm.c
+++ b/savevm.c
@@ -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);
     }

-- 
1.6.2.5

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

[Luiz Capitulino]

On Fri, 28 Aug 2009 22:31:57 +0200
Juan Quintela <quintela@redhat.com> wrote:

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  savevm.c |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index baef277..9836c60 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -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;

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

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

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Fri, 28 Aug 2009 22:31:57 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  savevm.c |   20 +++++++++++---------
>>  1 files changed, 11 insertions(+), 9 deletions(-)
>> 
>> diff --git a/savevm.c b/savevm.c
>> index baef277..9836c60 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -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;
>
>  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.

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

[Luiz Capitulino]

On Tue, 01 Sep 2009 00:08:43 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > On Fri, 28 Aug 2009 22:31:57 +0200
> > Juan Quintela <quintela@redhat.com> wrote:
> >
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  savevm.c |   20 +++++++++++---------
> >>  1 files changed, 11 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/savevm.c b/savevm.c
> >> index baef277..9836c60 100644
> >> --- a/savevm.c
> >> +++ b/savevm.c
> >> @@ -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;
> >
> >  You're missing the initialization here, spot this while
> > testing staging.
> 
> 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 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.

> #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.

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

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Tue, 01 Sep 2009 00:08:43 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> > On Fri, 28 Aug 2009 22:31:57 +0200
>> > Juan Quintela <quintela@redhat.com> wrote:
>> >
>> >> 
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >> ---
>> >>  savevm.c |   20 +++++++++++---------
>> >>  1 files changed, 11 insertions(+), 9 deletions(-)
>> >> 
>> >> diff --git a/savevm.c b/savevm.c
>> >> index baef277..9836c60 100644
>> >> --- a/savevm.c
>> >> +++ b/savevm.c
>> >> @@ -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;
>> >
>> >  You're missing the initialization here, spot this while
>> > testing staging.
>> 
>> 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.

>> 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.
>
>> #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.

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

[Gerd Hoffmann]

   Hi,

>   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