| Message ID | 1423064635-19045-8-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: > Fixes a QEMU crash when passing dump_guest_core parameter in command line. Explain that, please? > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > exec.c | 4 ++-- > hw/core/machine.c | 6 ++++++ > include/hw/boards.h | 1 + > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/exec.c b/exec.c > index 6b79ad1..bfca528 100644 > --- a/exec.c > +++ b/exec.c > @@ -26,6 +26,7 @@ > #include "cpu.h" > #include "tcg.h" > #include "hw/hw.h" > +#include "hw/boards.h" > #include "hw/qdev.h" > #include "qemu/osdep.h" > #include "sysemu/kvm.h" > @@ -1213,8 +1214,7 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t size) > int ret; > > /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */ > - if (!qemu_opt_get_bool(qemu_get_machine_opts(), > - "dump-guest-core", true)) { > + if (!machine_dump_guest_core(current_machine)) { linux-user doesn't have a current_machine, and machine.c shouldn't even be compiled in, so I'm guessing this function is #ifdef'ed, which means the header should be #ifdef'ed, too. hw/boards.h has no business being in *-user. Regards, Andreas > ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP); > if (ret) { > perror("qemu_madvise"); > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 5ad2409..8033683 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -285,6 +285,7 @@ static void machine_initfn(Object *obj) > > ms->kernel_irqchip_allowed = true; > ms->kvm_shadow_mem = -1; > + ms->dump_guest_core = true; > > object_property_add_str(obj, "accel", > machine_get_accel, machine_set_accel, NULL); > @@ -425,6 +426,11 @@ int machine_phandle_start(MachineState *machine) > return machine->phandle_start; > } > > +bool machine_dump_guest_core(MachineState *machine) > +{ > + return machine->dump_guest_core; > +} > + > static const TypeInfo machine_info = { > .name = TYPE_MACHINE, > .parent = TYPE_OBJECT, > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 1f21bdf..7de308e 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -71,6 +71,7 @@ bool machine_kernel_irqchip_allowed(MachineState *machine); > bool machine_kernel_irqchip_required(MachineState *machine); > int machine_kvm_shadow_mem(MachineState *machine); > int machine_phandle_start(MachineState *machine); > +bool machine_dump_guest_core(MachineState *machine); > > /** > * MachineClass: >
On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: > Hi, > > Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: > > Fixes a QEMU crash when passing dump_guest_core parameter in command line. > > Explain that, please? Pls note the submission date. It's 1 month late to ask for basic clarifications. I've merged the patches, I'll fix up issues such as prettifying includes by adding patches on top. > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > --- > > exec.c | 4 ++-- > > hw/core/machine.c | 6 ++++++ > > include/hw/boards.h | 1 + > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index 6b79ad1..bfca528 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -26,6 +26,7 @@ > > #include "cpu.h" > > #include "tcg.h" > > #include "hw/hw.h" > > +#include "hw/boards.h" > > #include "hw/qdev.h" > > #include "qemu/osdep.h" > > #include "sysemu/kvm.h" > > @@ -1213,8 +1214,7 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t size) > > int ret; > > > > /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */ > > - if (!qemu_opt_get_bool(qemu_get_machine_opts(), > > - "dump-guest-core", true)) { > > + if (!machine_dump_guest_core(current_machine)) { > > linux-user doesn't have a current_machine, and machine.c shouldn't even > be compiled in, so I'm guessing this function is #ifdef'ed, which means > the header should be #ifdef'ed, too. hw/boards.h has no business being > in *-user. > > Regards, > Andreas > > > ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP); > > if (ret) { > > perror("qemu_madvise"); > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 5ad2409..8033683 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -285,6 +285,7 @@ static void machine_initfn(Object *obj) > > > > ms->kernel_irqchip_allowed = true; > > ms->kvm_shadow_mem = -1; > > + ms->dump_guest_core = true; > > > > object_property_add_str(obj, "accel", > > machine_get_accel, machine_set_accel, NULL); > > @@ -425,6 +426,11 @@ int machine_phandle_start(MachineState *machine) > > return machine->phandle_start; > > } > > > > +bool machine_dump_guest_core(MachineState *machine) > > +{ > > + return machine->dump_guest_core; > > +} > > + > > static const TypeInfo machine_info = { > > .name = TYPE_MACHINE, > > .parent = TYPE_OBJECT, > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 1f21bdf..7de308e 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -71,6 +71,7 @@ bool machine_kernel_irqchip_allowed(MachineState *machine); > > bool machine_kernel_irqchip_required(MachineState *machine); > > int machine_kvm_shadow_mem(MachineState *machine); > > int machine_phandle_start(MachineState *machine); > > +bool machine_dump_guest_core(MachineState *machine); > > > > /** > > * MachineClass: > > > > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, > Graham Norton; HRB 21284 (AG Nürnberg)
Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: > On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: >> Hi, >> >> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: >>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. >> >> Explain that, please? > > Pls note the submission date. It's 1 month late to ask for > basic clarifications. > > I've merged the patches, I'll fix up issues such as prettifying > includes by adding patches on top. No, since the patch is not in qemu.git (it builds!) it is not too late to fix it, nor too late to ask why a patch that introduces a breakage does what it does. (Moving the info from the cover letter into the commit message would've been a good idea, Marcel.) All QEMU patches are supposed to be bisectable. It's our job as maintainers to build-test each. If you do that 1 month later, that's not my fault. Regards, Andreas
Andreas Färber <afaerber@suse.de> writes: > Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: >> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: >>> Hi, >>> >>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: >>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. >>> >>> Explain that, please? >> >> Pls note the submission date. It's 1 month late to ask for >> basic clarifications. >> >> I've merged the patches, I'll fix up issues such as prettifying >> includes by adding patches on top. > > No, since the patch is not in qemu.git (it builds!) it is not too late > to fix it, nor too late to ask why a patch that introduces a breakage > does what it does. Getting review that late is decidedly suboptimal, but no excuse to invoke maintainer privilege to ram the patch through unchanged. Cosmetic issues can be tidied up on top. The ongoing review may produce nothing but cosmetic issues, but we don't know that, yet. Commit messages can't be tidied up on top, and they're dirt cheap to improve right in place, so let's do that, please. [...]
On Wed, Mar 11, 2015 at 08:34:09AM +0100, Markus Armbruster wrote: > Andreas Färber <afaerber@suse.de> writes: > > > Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: > >> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: > >>> Hi, > >>> > >>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: > >>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. > >>> > >>> Explain that, please? > >> > >> Pls note the submission date. It's 1 month late to ask for > >> basic clarifications. > >> > >> I've merged the patches, I'll fix up issues such as prettifying > >> includes by adding patches on top. > > > > No, since the patch is not in qemu.git (it builds!) it is not too late > > to fix it, nor too late to ask why a patch that introduces a breakage > > does what it does. > > Getting review that late is decidedly suboptimal, but no excuse to > invoke maintainer privilege to ram the patch through unchanged. > > Cosmetic issues can be tidied up on top. The ongoing review may produce > nothing but cosmetic issues, but we don't know that, yet. Cool, review is good. What I wanted to say though is that I'm not holding up a patchset that's been around for a month just because of cosmetics and basic questions. So I intend to send pull request this evening - I don't think we want to live with known crashers any longer - crashes waste tester's time. > Commit messages can't be tidied up on top, and they're dirt cheap to > improve right in place, so let's do that, please. > > [...] Sure. Marcel, can you pls supply the command line that produces the crash? I'll include that.
On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote: > Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: > > On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: > >> Hi, > >> > >> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: > >>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. > >> > >> Explain that, please? > > > > Pls note the submission date. It's 1 month late to ask for > > basic clarifications. > > > > I've merged the patches, I'll fix up issues such as prettifying > > includes by adding patches on top. > > No, since the patch is not in qemu.git (it builds!) it is not too late > to fix it, nor too late to ask why a patch that introduces a breakage > does what it does. I tried to say that I'm not holding this patch set up because there are some basic questions. Paolo reviewed it and gave an ack. If others want to re-start review 1 month afterwards, that's fine, but I don't want to defer pull request with this any longer. If someone can quickly spot a serious non-cosmetic problem there, that's another matter, and would make me defer the pull request. > (Moving the info from the cover letter into the > commit message would've been a good idea, Marcel.) I can tweak commit messages, sure, since that does not require re-testing it all. > All QEMU patches are supposed to be bisectable. It's our job as > maintainers to build-test each. If you do that 1 month later, that's not > my fault. > > Regards, > Andreas I have this patch in my tree and there's no bisect issue, just test-built before and after this patch. That's because I had the ifdefs in boards.h which you and Peter objected to, but that is about cosmetics, I fixed that with a patch on top to hopefully make you both happy. Don't take my word for it, you can check out my tree and verify, that would be very wellcome. > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, > Graham Norton; HRB 21284 (AG Nürnberg)
On 03/11/2015 10:45 AM, Michael S. Tsirkin wrote: > On Wed, Mar 11, 2015 at 08:34:09AM +0100, Markus Armbruster wrote: >> Andreas Färber <afaerber@suse.de> writes: >> >>> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: >>>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: >>>>> Hi, >>>>> >>>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: >>>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. >>>>> >>>>> Explain that, please? >>>> >>>> Pls note the submission date. It's 1 month late to ask for >>>> basic clarifications. >>>> >>>> I've merged the patches, I'll fix up issues such as prettifying >>>> includes by adding patches on top. >>> >>> No, since the patch is not in qemu.git (it builds!) it is not too late >>> to fix it, nor too late to ask why a patch that introduces a breakage >>> does what it does. >> >> Getting review that late is decidedly suboptimal, but no excuse to >> invoke maintainer privilege to ram the patch through unchanged. >> >> Cosmetic issues can be tidied up on top. The ongoing review may produce >> nothing but cosmetic issues, but we don't know that, yet. > > Cool, review is good. What I wanted to say though is that I'm not > holding up a patchset that's been around for a month just because > of cosmetics and basic questions. > So I intend to send pull request this evening - I don't think we want to > live with known crashers any longer - crashes waste tester's time. > >> Commit messages can't be tidied up on top, and they're dirt cheap to >> improve right in place, so let's do that, please. >> >> [...] > > Sure. Marcel, can you pls supply the command line that > produces the crash? I'll include that. Sure you just have to use the option: qemu-bin ... -machine pc,dump-guest-core=on x86_64-softmmu/qemu-system-x86_64 -machine pc,dump-guest-core=on qemu-system-x86_64: qemu/util/qemu-option.c:387: qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. Aborted (core dumped) Thanks, Marcel >
On 03/10/2015 11:36 PM, Andreas Färber wrote: > Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: >> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: >>> Hi, >>> >>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: >>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. >>> >>> Explain that, please? >> >> Pls note the submission date. It's 1 month late to ask for >> basic clarifications. >> >> I've merged the patches, I'll fix up issues such as prettifying >> includes by adding patches on top. > > No, since the patch is not in qemu.git (it builds!) it is not too late > to fix it, nor too late to ask why a patch that introduces a breakage > does what it does. (Moving the info from the cover letter into the > commit message would've been a good idea, Marcel.) Hi Andreas, Agreed. Thanks, Marcel > > All QEMU patches are supposed to be bisectable. It's our job as > maintainers to build-test each. If you do that 1 month later, that's not > my fault. > > Regards, > Andreas >
Am 11.03.2015 um 09:56 schrieb Michael S. Tsirkin: > On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote: >> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: >>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: >>>> Hi, >>>> >>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: >>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. >>>> >>>> Explain that, please? >>> >>> Pls note the submission date. It's 1 month late to ask for >>> basic clarifications. >>> >>> I've merged the patches, I'll fix up issues such as prettifying >>> includes by adding patches on top. >> >> No, since the patch is not in qemu.git (it builds!) it is not too late >> to fix it, nor too late to ask why a patch that introduces a breakage >> does what it does. > > > I tried to say that I'm not holding this patch set up > because there are some basic questions. Paolo reviewed > it and gave an ack. If others want to re-start review 1 month > afterwards, that's fine, but I don't want to defer pull > request with this any longer. If someone can quickly spot > a serious non-cosmetic problem there, that's another > matter, and would make me defer the pull request. > > >> (Moving the info from the cover letter into the >> commit message would've been a good idea, Marcel.) > > I can tweak commit messages, sure, since that does not require > re-testing it all. > >> All QEMU patches are supposed to be bisectable. It's our job as >> maintainers to build-test each. If you do that 1 month later, that's not >> my fault. >> >> Regards, >> Andreas > > I have this patch in my tree and there's > no bisect issue, just test-built before and after this patch. > That's because I had the ifdefs in boards.h which you and > Peter objected to, but that is about cosmetics, I fixed that > with a patch on top to hopefully make you both happy. All I was asking for is, please squash the patch(es) that fix(es) the build issue. In particular if you applied the patch just yesterday when we complained. We've been required to, so I expect the same rules to apply to everyone. In order to propose a better fix I tried to understand what the patch is fixing, that's all. If an improvement of the commit message comes out of that, good, but that was not the main purpose. Thanks, Andreas P.S. I was sick most of February and my Chromebook has a broken DRM driver, not allowing for much bedside-hacking. ;) > > Don't take my word for it, you can check out my tree and verify, > that would be very wellcome. > >> -- >> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, >> Graham Norton; HRB 21284 (AG Nürnberg)
On 03/11/2015 01:06 PM, Andreas Färber wrote: > Am 11.03.2015 um 09:56 schrieb Michael S. Tsirkin: >> On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote: >>> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: >>>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: >>>>> Hi, >>>>> >>>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: >>>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. >>>>> >>>>> Explain that, please? >>>> >>>> Pls note the submission date. It's 1 month late to ask for >>>> basic clarifications. >>>> >>>> I've merged the patches, I'll fix up issues such as prettifying >>>> includes by adding patches on top. >>> >>> No, since the patch is not in qemu.git (it builds!) it is not too late >>> to fix it, nor too late to ask why a patch that introduces a breakage >>> does what it does. >> >> >> I tried to say that I'm not holding this patch set up >> because there are some basic questions. Paolo reviewed >> it and gave an ack. If others want to re-start review 1 month >> afterwards, that's fine, but I don't want to defer pull >> request with this any longer. If someone can quickly spot >> a serious non-cosmetic problem there, that's another >> matter, and would make me defer the pull request. >> >> >>> (Moving the info from the cover letter into the >>> commit message would've been a good idea, Marcel.) >> >> I can tweak commit messages, sure, since that does not require >> re-testing it all. >> >>> All QEMU patches are supposed to be bisectable. It's our job as >>> maintainers to build-test each. If you do that 1 month later, that's not >>> my fault. >>> >>> Regards, >>> Andreas >> >> I have this patch in my tree and there's >> no bisect issue, just test-built before and after this patch. >> That's because I had the ifdefs in boards.h which you and >> Peter objected to, but that is about cosmetics, I fixed that >> with a patch on top to hopefully make you both happy. > > All I was asking for is, please squash the patch(es) that fix(es) the > build issue. In particular if you applied the patch just yesterday when > we complained. We've been required to, so I expect the same rules to > apply to everyone. > > In order to propose a better fix I tried to understand what the patch is > fixing, that's all. If an improvement of the commit message comes out of > that, good, but that was not the main purpose. > > Thanks, > Andreas > > P.S. I was sick most of February and my Chromebook has a broken DRM > driver, not allowing for much bedside-hacking. ;) Hi Andreas, I hope you are feeling better now! The main issue I see here (and believe me is not the reviews, they are always welcomed!) is that more than a month ago several developers complained about these crashes. I stopped what I was doing and posted a series ASAP that was almost immediately reviewed by Paolo. I pinged twice already and nobody did anything about it. Michael took it because nobody else did and now we have a situation: "No good deed goes unpunished" Now, we need a way to not let this happen. I am afraid that next time I will not get lucky and nobody will take the patches :(. Thanks, Marcel > >> >> Don't take my word for it, you can check out my tree and verify, >> that would be very wellcome. >> >>> -- >>> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >>> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, >>> Graham Norton; HRB 21284 (AG Nürnberg) > >
On Wed, Mar 11, 2015 at 12:06:48PM +0100, Andreas Färber wrote: > Am 11.03.2015 um 09:56 schrieb Michael S. Tsirkin: > > On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote: > >> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: > >>> On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: > >>>> Hi, > >>>> > >>>> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: > >>>>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. > >>>> > >>>> Explain that, please? > >>> > >>> Pls note the submission date. It's 1 month late to ask for > >>> basic clarifications. > >>> > >>> I've merged the patches, I'll fix up issues such as prettifying > >>> includes by adding patches on top. > >> > >> No, since the patch is not in qemu.git (it builds!) it is not too late > >> to fix it, nor too late to ask why a patch that introduces a breakage > >> does what it does. > > > > > > I tried to say that I'm not holding this patch set up > > because there are some basic questions. Paolo reviewed > > it and gave an ack. If others want to re-start review 1 month > > afterwards, that's fine, but I don't want to defer pull > > request with this any longer. If someone can quickly spot > > a serious non-cosmetic problem there, that's another > > matter, and would make me defer the pull request. > > > > > >> (Moving the info from the cover letter into the > >> commit message would've been a good idea, Marcel.) > > > > I can tweak commit messages, sure, since that does not require > > re-testing it all. > > > >> All QEMU patches are supposed to be bisectable. It's our job as > >> maintainers to build-test each. If you do that 1 month later, that's not > >> my fault. > >> > >> Regards, > >> Andreas > > > > I have this patch in my tree and there's > > no bisect issue, just test-built before and after this patch. > > That's because I had the ifdefs in boards.h which you and > > Peter objected to, but that is about cosmetics, I fixed that > > with a patch on top to hopefully make you both happy. > > All I was asking for is, please squash the patch(es) that fix(es) the > build issue. Hmm as far as I can see, there's no build issue. My patch is required before Marcel's one. Then there's another one by me to address the comments you had. I could squash them all but this just messes up attribution, bisect build is fine, and I verified that. > In particular if you applied the patch just yesterday when > we complained. We've been required to, so I expect the same rules to > apply to everyone. > > In order to propose a better fix I tried to understand what the patch is > fixing, that's all. If an improvement of the commit message comes out of > that, good, but that was not the main purpose. > > Thanks, > Andreas > > P.S. I was sick most of February and my Chromebook has a broken DRM > driver, not allowing for much bedside-hacking. ;) I certainly didn't intend to blame anyone, sorry if it sounded like that. I was merely saying, it's been on review for a while, got ack and not objections, so let's apply it unless someone sees significant issues, I don't want to hold it up until all questions are answered. > > > > Don't take my word for it, you can check out my tree and verify, > > that would be very wellcome. > > > >> -- > >> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > >> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, > >> Graham Norton; HRB 21284 (AG Nürnberg) > > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, > Graham Norton; HRB 21284 (AG Nürnberg)
On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote: > Fixes a QEMU crash when passing dump_guest_core parameter in command line. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> Please amend commit message: Running qemu-bin ... -machine pc,dump-guest-core=on leads to crash: x86_64-softmmu/qemu-system-x86_64 -machine pc,dump-guest-core=on qemu-system-x86_64: qemu/util/qemu-option.c:387: qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. Aborted (core dumped) This happens because the commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed the global option descriptions and moved them to MachineState's QOM properties. Fix this by querying machine properties through designated wrappers. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > exec.c | 4 ++-- > hw/core/machine.c | 6 ++++++ > include/hw/boards.h | 1 + > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/exec.c b/exec.c > index 6b79ad1..bfca528 100644 > --- a/exec.c > +++ b/exec.c > @@ -26,6 +26,7 @@ > #include "cpu.h" > #include "tcg.h" > #include "hw/hw.h" > +#include "hw/boards.h" > #include "hw/qdev.h" > #include "qemu/osdep.h" > #include "sysemu/kvm.h" > @@ -1213,8 +1214,7 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t size) > int ret; > > /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */ > - if (!qemu_opt_get_bool(qemu_get_machine_opts(), > - "dump-guest-core", true)) { > + if (!machine_dump_guest_core(current_machine)) { > ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP); > if (ret) { > perror("qemu_madvise"); > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 5ad2409..8033683 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -285,6 +285,7 @@ static void machine_initfn(Object *obj) > > ms->kernel_irqchip_allowed = true; > ms->kvm_shadow_mem = -1; > + ms->dump_guest_core = true; > > object_property_add_str(obj, "accel", > machine_get_accel, machine_set_accel, NULL); > @@ -425,6 +426,11 @@ int machine_phandle_start(MachineState *machine) > return machine->phandle_start; > } > > +bool machine_dump_guest_core(MachineState *machine) > +{ > + return machine->dump_guest_core; > +} > + > static const TypeInfo machine_info = { > .name = TYPE_MACHINE, > .parent = TYPE_OBJECT, > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 1f21bdf..7de308e 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -71,6 +71,7 @@ bool machine_kernel_irqchip_allowed(MachineState *machine); > bool machine_kernel_irqchip_required(MachineState *machine); > int machine_kvm_shadow_mem(MachineState *machine); > int machine_phandle_start(MachineState *machine); > +bool machine_dump_guest_core(MachineState *machine); > > /** > * MachineClass: >
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Tue, Mar 10, 2015 at 10:36:56PM +0100, Andreas Färber wrote: >> Am 10.03.2015 um 22:24 schrieb Michael S. Tsirkin: >> > On Tue, Mar 10, 2015 at 06:50:24PM +0100, Andreas Färber wrote: >> >> Hi, >> >> >> >> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: >> >>> Fixes a QEMU crash when passing dump_guest_core parameter in command line. >> >> >> >> Explain that, please? >> > >> > Pls note the submission date. It's 1 month late to ask for >> > basic clarifications. >> > >> > I've merged the patches, I'll fix up issues such as prettifying >> > includes by adding patches on top. >> >> No, since the patch is not in qemu.git (it builds!) it is not too late >> to fix it, nor too late to ask why a patch that introduces a breakage >> does what it does. > > > I tried to say that I'm not holding this patch set up > because there are some basic questions. Paolo reviewed > it and gave an ack. If others want to re-start review 1 month > afterwards, that's fine, but I don't want to defer pull > request with this any longer. If someone can quickly spot > a serious non-cosmetic problem there, that's another > matter, and would make me defer the pull request. Okay, sounds much more reasonable than your first reply. Thanks for clarifying. [...]
diff --git a/exec.c b/exec.c index 6b79ad1..bfca528 100644 --- a/exec.c +++ b/exec.c @@ -26,6 +26,7 @@ #include "cpu.h" #include "tcg.h" #include "hw/hw.h" +#include "hw/boards.h" #include "hw/qdev.h" #include "qemu/osdep.h" #include "sysemu/kvm.h" @@ -1213,8 +1214,7 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t size) int ret; /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */ - if (!qemu_opt_get_bool(qemu_get_machine_opts(), - "dump-guest-core", true)) { + if (!machine_dump_guest_core(current_machine)) { ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP); if (ret) { perror("qemu_madvise"); diff --git a/hw/core/machine.c b/hw/core/machine.c index 5ad2409..8033683 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -285,6 +285,7 @@ static void machine_initfn(Object *obj) ms->kernel_irqchip_allowed = true; ms->kvm_shadow_mem = -1; + ms->dump_guest_core = true; object_property_add_str(obj, "accel", machine_get_accel, machine_set_accel, NULL); @@ -425,6 +426,11 @@ int machine_phandle_start(MachineState *machine) return machine->phandle_start; } +bool machine_dump_guest_core(MachineState *machine) +{ + return machine->dump_guest_core; +} + static const TypeInfo machine_info = { .name = TYPE_MACHINE, .parent = TYPE_OBJECT, diff --git a/include/hw/boards.h b/include/hw/boards.h index 1f21bdf..7de308e 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -71,6 +71,7 @@ bool machine_kernel_irqchip_allowed(MachineState *machine); bool machine_kernel_irqchip_required(MachineState *machine); int machine_kvm_shadow_mem(MachineState *machine); int machine_phandle_start(MachineState *machine); +bool machine_dump_guest_core(MachineState *machine); /** * MachineClass:
Fixes a QEMU crash when passing dump_guest_core parameter in command line. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- exec.c | 4 ++-- hw/core/machine.c | 6 ++++++ include/hw/boards.h | 1 + 3 files changed, 9 insertions(+), 2 deletions(-)