Bring savevm.c to the wonderful world of sys-queue.h
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(-)
PATCH 1/3 savevm: Convert savevm handlers list to TAILQ
--- savevm.c | 94 +++++++++++++++++++++++++++---------------------------------- 1 files changed, 42 insertions(+), 52 deletions(-)
savevm.c +42 -52
@@ -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
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
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
@@ -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
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
@@ -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
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:
Signed-off-by Juan Quintela <[email protected]>--- savevm.c | 20 +++++++++++--------- 1 files changed, 11 insertions(+), 9 deletions(-)savevm.c +1
-1@@ -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.
[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:
Signed-off-by Juan Quintela <[email protected]>--- savevm.c | 20 +++++++++++--------- 1 files changed, 11 insertions(+), 9 deletions(-)savevm.c +1
-1@@ -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
-1int 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
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:
Signed-off-by Juan Quintela <[email protected]>--- savevm.c | 20 +++++++++++--------- 1 files changed, 11 insertions(+), 9 deletions(-)savevm.c +1
-1@@ -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
-1int 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.cand guess what :) NoLIST_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.
[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:
Signed-off-by Juan Quintela <[email protected]>--- savevm.c | 20 +++++++++++--------- 1 files changed, 11 insertions(+), 9 deletions(-)savevm.c +1
-1@@ -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.cand guess what :) NoLIST_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.