| Message ID | 39651e275488caec46861cb5b11ba1c7961a429c.1340910651.git.quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 28.06.2012 21:21, schrieb Juan Quintela: > This would make easier to add more operations in the next patches. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > savevm.c | 54 +++++++++++++++++++++++++----------------------------- > vmstate.h | 7 +++++++ > 2 files changed, 32 insertions(+), 29 deletions(-) > Hi Juan, this mail and the other mails from this patch series trigger an alert in my mail filter: X-Amavis-Alert: BAD HEADER SECTION Duplicate header field: "References" The following extract from the mail header shows that there is really a duplication of the fields "In-Reply-To" and "References": From: Juan Quintela <quintela@redhat.com> To: qemu-devel@nongnu.org Date: Thu, 28 Jun 2012 21:21:59 +0200 Message-Id: <39651e275488caec46861cb5b11ba1c7961a429c.1340910651.git.quintela@redhat.com> In-Reply-To: <cover.1340910651.git.quintela@redhat.com> References: <cover.1340910651.git.quintela@redhat.com> In-Reply-To: <cover.1340910651.git.quintela@redhat.com> References: <cover.1340910651.git.quintela@redhat.com> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.12 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: owasserm@redhat.com Subject: [Qemu-devel] [PATCH 01/12] savevm: Use a struct to pass all handlers This is not a new issue, and there are lots of other mail senders which also send mails with duplications. I noticed that recently and would like to understand how it happens. Maybe some tool (git-send-email?) which is used for sending mails is buggy. Do you remember how you created the patch mails? Which mailer did you use? Do you also see the duplication in your local copy of that mail? Do other users also see the duplicate lines in the full mail header? Or is this a bug in my MTA? Regards, Stefan W.
On 06/28/2012 10:21 PM, Juan Quintela wrote: > This would make easier to add more operations in the next patches. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > savevm.c | 54 +++++++++++++++++++++++++----------------------------- > vmstate.h | 7 +++++++ > 2 files changed, 32 insertions(+), 29 deletions(-) > > diff --git a/savevm.c b/savevm.c > index a15c163..73626d4 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1171,10 +1171,7 @@ typedef struct SaveStateEntry { > int alias_id; > int version_id; > int section_id; > - SaveSetParamsHandler *set_params; > - SaveLiveStateHandler *save_live_state; > - SaveStateHandler *save_state; > - LoadStateHandler *load_state; > + SaveVMHandlers *ops; > const VMStateDescription *vmsd; > void *opaque; > CompatEntry *compat; > @@ -1237,10 +1234,11 @@ int register_savevm_live(DeviceState *dev, > se = g_malloc0(sizeof(SaveStateEntry)); > se->version_id = version_id; > se->section_id = global_section_id++; > - se->set_params = set_params; > - se->save_live_state = save_live_state; > - se->save_state = save_state; > - se->load_state = load_state; > + se->ops = g_malloc0(sizeof(SaveVMHandlers)); > + se->ops->set_params = set_params; > + se->ops->save_live_state = save_live_state; > + se->ops->save_state = save_state; > + se->ops->load_state = load_state; > se->opaque = opaque; > se->vmsd = NULL; > se->no_migrate = 0; > @@ -1309,6 +1307,7 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque) > if (se->compat) { > g_free(se->compat); > } > + g_free(se->ops); > g_free(se); > } > } > @@ -1327,9 +1326,6 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, > se = g_malloc0(sizeof(SaveStateEntry)); > se->version_id = vmsd->version_id; > se->section_id = global_section_id++; > - se->save_live_state = NULL; > - se->save_state = NULL; > - se->load_state = NULL; > se->opaque = opaque; > se->vmsd = vmsd; > se->alias_id = alias_id; > @@ -1524,7 +1520,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, > static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) > { > if (!se->vmsd) { /* Old style */ > - return se->load_state(f, se->opaque, version_id); > + return se->ops->load_state(f, se->opaque, version_id); > } > return vmstate_load_state(f, se->vmsd, se->opaque, version_id); > } > @@ -1532,7 +1528,7 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) > static void vmstate_save(QEMUFile *f, SaveStateEntry *se) > { > if (!se->vmsd) { /* Old style */ > - se->save_state(f, se->opaque); > + se->ops->save_state(f, se->opaque); > return; > } > vmstate_save_state(f,se->vmsd, se->opaque); > @@ -1569,10 +1565,10 @@ int qemu_savevm_state_begin(QEMUFile *f, > int ret; > > QTAILQ_FOREACH(se, &savevm_handlers, entry) { > - if(se->set_params == NULL) { > + if (!se->ops || !se->ops->set_params) { > continue; > } > - se->set_params(params, se->opaque); > + se->ops->set_params(params, se->opaque); > } > > qemu_put_be32(f, QEMU_VM_FILE_MAGIC); > @@ -1581,9 +1577,9 @@ int qemu_savevm_state_begin(QEMUFile *f, > QTAILQ_FOREACH(se, &savevm_handlers, entry) { > int len; > > - if (se->save_live_state == NULL) > + if (!se->ops || !se->ops->save_live_state) { > continue; > - > + } > /* Section type */ > qemu_put_byte(f, QEMU_VM_SECTION_START); > qemu_put_be32(f, se->section_id); > @@ -1596,7 +1592,7 @@ int qemu_savevm_state_begin(QEMUFile *f, > qemu_put_be32(f, se->instance_id); > qemu_put_be32(f, se->version_id); > > - ret = se->save_live_state(f, QEMU_VM_SECTION_START, se->opaque); > + ret = se->ops->save_live_state(f, QEMU_VM_SECTION_START, se->opaque); > if (ret < 0) { > qemu_savevm_state_cancel(f); > return ret; > @@ -1623,9 +1619,9 @@ int qemu_savevm_state_iterate(QEMUFile *f) > int ret = 1; > > QTAILQ_FOREACH(se, &savevm_handlers, entry) { > - if (se->save_live_state == NULL) > + if (!se->ops || !se->ops->save_live_state) { > continue; > - > + } > if (qemu_file_rate_limit(f)) { > return 0; > } > @@ -1634,7 +1630,7 @@ int qemu_savevm_state_iterate(QEMUFile *f) > qemu_put_byte(f, QEMU_VM_SECTION_PART); > qemu_put_be32(f, se->section_id); > > - ret = se->save_live_state(f, QEMU_VM_SECTION_PART, se->opaque); > + ret = se->ops->save_live_state(f, QEMU_VM_SECTION_PART, se->opaque); > trace_savevm_section_end(se->section_id); > > if (ret <= 0) { > @@ -1663,15 +1659,15 @@ int qemu_savevm_state_complete(QEMUFile *f) > cpu_synchronize_all_states(); > > QTAILQ_FOREACH(se, &savevm_handlers, entry) { > - if (se->save_live_state == NULL) > + if (!se->ops || !se->ops->save_live_state) { > continue; > - > + } > trace_savevm_section_start(); > /* Section type */ > qemu_put_byte(f, QEMU_VM_SECTION_END); > qemu_put_be32(f, se->section_id); > > - ret = se->save_live_state(f, QEMU_VM_SECTION_END, se->opaque); > + ret = se->ops->save_live_state(f, QEMU_VM_SECTION_END, se->opaque); > trace_savevm_section_end(se->section_id); > if (ret < 0) { > return ret; > @@ -1681,9 +1677,9 @@ int qemu_savevm_state_complete(QEMUFile *f) > QTAILQ_FOREACH(se, &savevm_handlers, entry) { > int len; > > - if (se->save_state == NULL && se->vmsd == NULL) > + if ((!se->ops || !se->ops->save_state) && !se->vmsd) { > continue; > - > + } > trace_savevm_section_start(); > /* Section type */ > qemu_put_byte(f, QEMU_VM_SECTION_FULL); > @@ -1711,8 +1707,8 @@ void qemu_savevm_state_cancel(QEMUFile *f) > SaveStateEntry *se; > > QTAILQ_FOREACH(se, &savevm_handlers, entry) { > - if (se->save_live_state) { > - se->save_live_state(f, -1, se->opaque); > + if (se->ops && se->ops->save_live_state) { > + se->ops->save_live_state(f, -1, se->opaque); > } > } > } > @@ -1765,7 +1761,7 @@ static int qemu_save_device_state(QEMUFile *f) > if (se->is_ram) { > continue; > } > - if (se->save_state == NULL && se->vmsd == NULL) { > + if ((!se->ops || !se->ops->save_state) && !se->vmsd) { > continue; > } > > diff --git a/vmstate.h b/vmstate.h > index 5af45e0..909af69 100644 > --- a/vmstate.h > +++ b/vmstate.h > @@ -31,6 +31,13 @@ typedef void SaveStateHandler(QEMUFile *f, void *opaque); > typedef int SaveLiveStateHandler(QEMUFile *f, int stage, void *opaque); > typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id); > > +typedef struct SaveVMHandlers { > + SaveSetParamsHandler *set_params; > + SaveStateHandler *save_state; > + SaveLiveStateHandler *save_live_state; > + LoadStateHandler *load_state; > +} SaveVMHandlers; > + > int register_savevm(DeviceState *dev, > const char *idstr, > int instance_id, > Reviewed-by: Orit Wasserman <owasserm@redhat.com>
Stefan Weil <sw@weilnetz.de> wrote: > Am 28.06.2012 21:21, schrieb Juan Quintela: >> This would make easier to add more operations in the next patches. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> savevm.c | 54 +++++++++++++++++++++++++----------------------------- >> vmstate.h | 7 +++++++ >> 2 files changed, 32 insertions(+), 29 deletions(-) >> > > > Hi Juan, Hi stefan > > this mail and the other mails from this patch series trigger > an alert in my mail filter: Just before we start, I just hate email (TM) :-(. > X-Amavis-Alert: BAD HEADER SECTION Duplicate header field: "References" I am looking, at the patches generated by "gfpm" alias just have one of each. Looking into my INBOX. You are right, I also have duplicate In-Reply-To and Reference fields. No clue if the problem is with git or mailer. If it makes any difference, this is git: (trasno *)$ rpm -qa git git-1.7.10.4-1.fc17.x86_64 > The following extract from the mail header shows that there is really > a duplication of the fields "In-Reply-To" and "References": > > From: Juan Quintela <quintela@redhat.com> > To: qemu-devel@nongnu.org > Date: Thu, 28 Jun 2012 21:21:59 +0200 > Message-Id: > <39651e275488caec46861cb5b11ba1c7961a429c.1340910651.git.quintela@redhat.com> > In-Reply-To: <cover.1340910651.git.quintela@redhat.com> > References: <cover.1340910651.git.quintela@redhat.com> > In-Reply-To: <cover.1340910651.git.quintela@redhat.com> > References: <cover.1340910651.git.quintela@redhat.com> > X-Scanned-By: MIMEDefang 2.67 on 10.5.11.12 > X-detected-operating-system: by eggs.gnu.org: Genre and OS details not > recognized. > X-Received-From: 209.132.183.28 > Cc: owasserm@redhat.com > Subject: [Qemu-devel] [PATCH 01/12] savevm: Use a struct to pass all > handlers > > This is not a new issue, and there are lots of other mail senders which > also send mails with duplications. I noticed that recently and would like > to understand how it happens. Maybe some tool (git-send-email?) which is > used for sending mails is buggy. > > Do you remember how you created the patch mails? alias gfpm='/usr/bin/git format-patch --signoff --find-renames --thread--cover-letter alias gsend-qemu='/usr/bin/git send-email --smtp-server=smtp.corp.redhat.com --bcc=quintela@redhat.com --from="Juan Quintela <quintela@redhat.com>" --suppress-cc=self --to=qemu-devel@nongnu.org' cd $QEMU_DIR gfpm master -o /tmp/folder/ <edit cover letter until happy> gsend-qemu /tmp/folder/*patch I have always send the emails this way (this is why I have the alias). > Which mailer did you use? Command line, redhat server is zimbra (I think it has postfix underneath, but I would not trust my memory so much). > Do you also see the duplication in your local copy of that mail? As said, the generated file with git-format-patch, only have one header field of each, but the mail that I receive have it duplicated. > Do other users also see the duplicate lines in the full mail header? > > Or is this a bug in my MTA? I also see them, just that I don't get the warning from my mailer. Anyone has any clue? I think that the commands that I use are the "normal" way to send multi-patches series? And what is more, nobody complained in the past. > Regards, Later, Juan.
On 1 July 2012 11:16, Juan Quintela <quintela@redhat.com> wrote: > Stefan Weil <sw@weilnetz.de> wrote: >> this mail and the other mails from this patch series trigger >> an alert in my mail filter: >> X-Amavis-Alert: BAD HEADER SECTION Duplicate header field: "References" > You are right, I also have duplicate In-Reply-To and Reference fields. > No clue if the problem is with git or mailer. It seems to have been reported as a Debian bug against git a while back: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=593542 but the git-send-email manpage suggests that upstream would like to consider this as user error: # It is up to the user to ensure that no In-Reply-To header already # exists when git send-email is asked to add it (especially note that # git format-patch can be configured to do the threading itself). # Failure to do so may not produce the expected result in the # recipient’s MUA. I think what this boils down to is "don't tell both format-patch and send-email to do threading". send-email threads by default, and format-patch doesn't thread by default, so the path of least resistance is probably not to pass '--thread' to format-patch. (Alternatively you could tell format-patch --thread and send-email --no-thread, but I don't know why you'd want to do that that way around.) -- PMM
Peter Maydell <peter.maydell@linaro.org> wrote: > On 1 July 2012 11:16, Juan Quintela <quintela@redhat.com> wrote: >> Stefan Weil <sw@weilnetz.de> wrote: >>> this mail and the other mails from this patch series trigger >>> an alert in my mail filter: >>> X-Amavis-Alert: BAD HEADER SECTION Duplicate header field: "References" > >> You are right, I also have duplicate In-Reply-To and Reference fields. >> No clue if the problem is with git or mailer. Thanks Peter, it appears that now problem is fixed. For reference: alias gfpm="/usr/bin/git format-patch --signoff --find-renames --cover-letter" alias gsend="/usr/bin/git send-email --smtp-server=smtp.corp.redhat.com --bcc=quintela@redhat.com --from=\"Juan Quintela <quintela@redhat.com>\" --suppress-cc=self" i.e. just remove the --thread from gfpm. Thanks to both, Juan.
diff --git a/savevm.c b/savevm.c index a15c163..73626d4 100644 --- a/savevm.c +++ b/savevm.c @@ -1171,10 +1171,7 @@ typedef struct SaveStateEntry { int alias_id; int version_id; int section_id; - SaveSetParamsHandler *set_params; - SaveLiveStateHandler *save_live_state; - SaveStateHandler *save_state; - LoadStateHandler *load_state; + SaveVMHandlers *ops; const VMStateDescription *vmsd; void *opaque; CompatEntry *compat; @@ -1237,10 +1234,11 @@ int register_savevm_live(DeviceState *dev, se = g_malloc0(sizeof(SaveStateEntry)); se->version_id = version_id; se->section_id = global_section_id++; - se->set_params = set_params; - se->save_live_state = save_live_state; - se->save_state = save_state; - se->load_state = load_state; + se->ops = g_malloc0(sizeof(SaveVMHandlers)); + se->ops->set_params = set_params; + se->ops->save_live_state = save_live_state; + se->ops->save_state = save_state; + se->ops->load_state = load_state; se->opaque = opaque; se->vmsd = NULL; se->no_migrate = 0; @@ -1309,6 +1307,7 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque) if (se->compat) { g_free(se->compat); } + g_free(se->ops); g_free(se); } } @@ -1327,9 +1326,6 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, se = g_malloc0(sizeof(SaveStateEntry)); se->version_id = vmsd->version_id; se->section_id = global_section_id++; - se->save_live_state = NULL; - se->save_state = NULL; - se->load_state = NULL; se->opaque = opaque; se->vmsd = vmsd; se->alias_id = alias_id; @@ -1524,7 +1520,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) { if (!se->vmsd) { /* Old style */ - return se->load_state(f, se->opaque, version_id); + return se->ops->load_state(f, se->opaque, version_id); } return vmstate_load_state(f, se->vmsd, se->opaque, version_id); } @@ -1532,7 +1528,7 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) static void vmstate_save(QEMUFile *f, SaveStateEntry *se) { if (!se->vmsd) { /* Old style */ - se->save_state(f, se->opaque); + se->ops->save_state(f, se->opaque); return; } vmstate_save_state(f,se->vmsd, se->opaque); @@ -1569,10 +1565,10 @@ int qemu_savevm_state_begin(QEMUFile *f, int ret; QTAILQ_FOREACH(se, &savevm_handlers, entry) { - if(se->set_params == NULL) { + if (!se->ops || !se->ops->set_params) { continue; } - se->set_params(params, se->opaque); + se->ops->set_params(params, se->opaque); } qemu_put_be32(f, QEMU_VM_FILE_MAGIC); @@ -1581,9 +1577,9 @@ int qemu_savevm_state_begin(QEMUFile *f, QTAILQ_FOREACH(se, &savevm_handlers, entry) { int len; - if (se->save_live_state == NULL) + if (!se->ops || !se->ops->save_live_state) { continue; - + } /* Section type */ qemu_put_byte(f, QEMU_VM_SECTION_START); qemu_put_be32(f, se->section_id); @@ -1596,7 +1592,7 @@ int qemu_savevm_state_begin(QEMUFile *f, qemu_put_be32(f, se->instance_id); qemu_put_be32(f, se->version_id); - ret = se->save_live_state(f, QEMU_VM_SECTION_START, se->opaque); + ret = se->ops->save_live_state(f, QEMU_VM_SECTION_START, se->opaque); if (ret < 0) { qemu_savevm_state_cancel(f); return ret; @@ -1623,9 +1619,9 @@ int qemu_savevm_state_iterate(QEMUFile *f) int ret = 1; QTAILQ_FOREACH(se, &savevm_handlers, entry) { - if (se->save_live_state == NULL) + if (!se->ops || !se->ops->save_live_state) { continue; - + } if (qemu_file_rate_limit(f)) { return 0; } @@ -1634,7 +1630,7 @@ int qemu_savevm_state_iterate(QEMUFile *f) qemu_put_byte(f, QEMU_VM_SECTION_PART); qemu_put_be32(f, se->section_id); - ret = se->save_live_state(f, QEMU_VM_SECTION_PART, se->opaque); + ret = se->ops->save_live_state(f, QEMU_VM_SECTION_PART, se->opaque); trace_savevm_section_end(se->section_id); if (ret <= 0) { @@ -1663,15 +1659,15 @@ int qemu_savevm_state_complete(QEMUFile *f) cpu_synchronize_all_states(); QTAILQ_FOREACH(se, &savevm_handlers, entry) { - if (se->save_live_state == NULL) + if (!se->ops || !se->ops->save_live_state) { continue; - + } trace_savevm_section_start(); /* Section type */ qemu_put_byte(f, QEMU_VM_SECTION_END); qemu_put_be32(f, se->section_id); - ret = se->save_live_state(f, QEMU_VM_SECTION_END, se->opaque); + ret = se->ops->save_live_state(f, QEMU_VM_SECTION_END, se->opaque); trace_savevm_section_end(se->section_id); if (ret < 0) { return ret; @@ -1681,9 +1677,9 @@ int qemu_savevm_state_complete(QEMUFile *f) QTAILQ_FOREACH(se, &savevm_handlers, entry) { int len; - if (se->save_state == NULL && se->vmsd == NULL) + if ((!se->ops || !se->ops->save_state) && !se->vmsd) { continue; - + } trace_savevm_section_start(); /* Section type */ qemu_put_byte(f, QEMU_VM_SECTION_FULL); @@ -1711,8 +1707,8 @@ void qemu_savevm_state_cancel(QEMUFile *f) SaveStateEntry *se; QTAILQ_FOREACH(se, &savevm_handlers, entry) { - if (se->save_live_state) { - se->save_live_state(f, -1, se->opaque); + if (se->ops && se->ops->save_live_state) { + se->ops->save_live_state(f, -1, se->opaque); } } } @@ -1765,7 +1761,7 @@ static int qemu_save_device_state(QEMUFile *f) if (se->is_ram) { continue; } - if (se->save_state == NULL && se->vmsd == NULL) { + if ((!se->ops || !se->ops->save_state) && !se->vmsd) { continue; } diff --git a/vmstate.h b/vmstate.h index 5af45e0..909af69 100644 --- a/vmstate.h +++ b/vmstate.h @@ -31,6 +31,13 @@ typedef void SaveStateHandler(QEMUFile *f, void *opaque); typedef int SaveLiveStateHandler(QEMUFile *f, int stage, void *opaque); typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id); +typedef struct SaveVMHandlers { + SaveSetParamsHandler *set_params; + SaveStateHandler *save_state; + SaveLiveStateHandler *save_live_state; + LoadStateHandler *load_state; +} SaveVMHandlers; + int register_savevm(DeviceState *dev, const char *idstr, int instance_id,
This would make easier to add more operations in the next patches. Signed-off-by: Juan Quintela <quintela@redhat.com> --- savevm.c | 54 +++++++++++++++++++++++++----------------------------- vmstate.h | 7 +++++++ 2 files changed, 32 insertions(+), 29 deletions(-)