| Message ID | c9dabedcdce60a1883531a9808e8fcbb72545456.1251491268.git.quintela@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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.
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.
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.
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.
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); }
Signed-off-by: Juan Quintela <quintela@redhat.com> --- savevm.c | 20 +++++++++++--------- 1 files changed, 11 insertions(+), 9 deletions(-)