| Message ID | 1377103396-24307-4-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 21, 2013 at 06:43:16PM +0200, Paolo Bonzini wrote: > The pvpanic situation is already messed up enough. Let us give our > libvirt friends an easy indication that we have untied our side. > > Not-yet-signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > ... because we first have to determine how to expose the device's existence > in the ACPI tables or in fw_cfg. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> So it's isa-pvpanic meaning "I-am-sure-this-is-the-last-bug-pvpanic" If we feel there's need to give libvirt a way to do introspection into QEMU bugs, let's architect one. Randomly renaming devices in the vain hope it's the last major bug is not it. NACK > --- > hw/misc/pvpanic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > index 1928cc9..b53011d 100644 > --- a/hw/misc/pvpanic.c > +++ b/hw/misc/pvpanic.c > @@ -27,7 +27,7 @@ > /* The pv event value */ > #define PVPANIC_PANICKED (1 << PVPANIC_F_PANICKED) > > -#define TYPE_ISA_PVPANIC_DEVICE "pvpanic" > +#define TYPE_ISA_PVPANIC_DEVICE "isa-pvpanic" > #define ISA_PVPANIC_DEVICE(obj) \ > OBJECT_CHECK(PVPanicState, (obj), TYPE_ISA_PVPANIC_DEVICE) > > -- > 1.8.3.1
Il 21/08/2013 19:01, Michael S. Tsirkin ha scritto: >> > The pvpanic situation is already messed up enough. Let us give our >> > libvirt friends an easy indication that we have untied our side. >> > >> > Not-yet-signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> > ... because we first have to determine how to expose the device's existence >> > in the ACPI tables or in fw_cfg. >> > >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > So it's isa-pvpanic meaning "I-am-sure-this-is-the-last-bug-pvpanic" More like "we tested pvpanic for more than 2 weeks and did not find anything that's utterly broken in the design". And more practically "you are sure there are no traces of builtin pvpanic; also, panicked state is reversible". Paolo > If we feel there's need to give libvirt a way to do > introspection into QEMU bugs, let's architect one. > Randomly renaming devices in the vain hope it's > the last major bug is not it.
Il 21/08/2013 19:07, Michael S. Tsirkin ha scritto: > On Wed, Aug 21, 2013 at 07:01:39PM +0200, Paolo Bonzini wrote: >> Il 21/08/2013 19:01, Michael S. Tsirkin ha scritto: >>>>> The pvpanic situation is already messed up enough. Let us give our >>>>> libvirt friends an easy indication that we have untied our side. >>>>> >>>>> Not-yet-signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>>>> ... because we first have to determine how to expose the device's existence >>>>> in the ACPI tables or in fw_cfg. >>>>> >>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> So it's isa-pvpanic meaning "I-am-sure-this-is-the-last-bug-pvpanic" >> >> More like "we tested pvpanic for more than 2 weeks and did not find >> anything that's utterly broken in the design". >> >> And more practically "you are sure there are no traces of builtin >> pvpanic; also, panicked state is reversible". > > isa-pvpanic does not look like a sane way to say that. > > NACK You know that a single developer's NACK counts nothing (it can be you, it can be me), don't you? Paolo
On Wed, Aug 21, 2013 at 07:01:39PM +0200, Paolo Bonzini wrote: > Il 21/08/2013 19:01, Michael S. Tsirkin ha scritto: > >> > The pvpanic situation is already messed up enough. Let us give our > >> > libvirt friends an easy indication that we have untied our side. > >> > > >> > Not-yet-signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> > ... because we first have to determine how to expose the device's existence > >> > in the ACPI tables or in fw_cfg. > >> > > >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > So it's isa-pvpanic meaning "I-am-sure-this-is-the-last-bug-pvpanic" > > More like "we tested pvpanic for more than 2 weeks and did not find > anything that's utterly broken in the design". > > And more practically "you are sure there are no traces of builtin > pvpanic; also, panicked state is reversible". > > Paolo isa-pvpanic does not look like a sane way to say that. NACK > > If we feel there's need to give libvirt a way to do > > introspection into QEMU bugs, let's architect one. > > Randomly renaming devices in the vain hope it's > > the last major bug is not it.
On Wed, Aug 21, 2013 at 07:06:21PM +0200, Paolo Bonzini wrote: > Il 21/08/2013 19:07, Michael S. Tsirkin ha scritto: > > On Wed, Aug 21, 2013 at 07:01:39PM +0200, Paolo Bonzini wrote: > >> Il 21/08/2013 19:01, Michael S. Tsirkin ha scritto: > >>>>> The pvpanic situation is already messed up enough. Let us give our > >>>>> libvirt friends an easy indication that we have untied our side. > >>>>> > >>>>> Not-yet-signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >>>>> ... because we first have to determine how to expose the device's existence > >>>>> in the ACPI tables or in fw_cfg. > >>>>> > >>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >>> So it's isa-pvpanic meaning "I-am-sure-this-is-the-last-bug-pvpanic" > >> > >> More like "we tested pvpanic for more than 2 weeks and did not find > >> anything that's utterly broken in the design". > >> > >> And more practically "you are sure there are no traces of builtin > >> pvpanic; also, panicked state is reversible". > > > > isa-pvpanic does not look like a sane way to say that. > > > > NACK > > You know that a single developer's NACK counts nothing (it can be you, > it can be me), don't you? > > Paolo No I don't.
Am 21.08.2013 19:01, schrieb Michael S. Tsirkin: > On Wed, Aug 21, 2013 at 06:43:16PM +0200, Paolo Bonzini wrote: >> The pvpanic situation is already messed up enough. Let us give our >> libvirt friends an easy indication that we have untied our side. >> >> Not-yet-signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> ... because we first have to determine how to expose the device's existence >> in the ACPI tables or in fw_cfg. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > So it's isa-pvpanic meaning "I-am-sure-this-is-the-last-bug-pvpanic" > > If we feel there's need to give libvirt a way to do > introspection into QEMU bugs, let's architect one. > Randomly renaming devices in the vain hope it's > the last major bug is not it. > > NACK Seconded. While we shouldn't rule out renaming devices, doing so as a criteria for libvirt sounds utterly wrong. Paolo, you are right that a single "NACK" cannot be a criteria, but a single convincing justification by a random reviewer should be sufficient to reconsider. :) Adding some device property or obtaining the info via some existing query-* QMP command might be better alternatives. Andreas
Il 21/08/2013 19:35, Andreas Färber ha scritto: > Am 21.08.2013 19:01, schrieb Michael S. Tsirkin: >> On Wed, Aug 21, 2013 at 06:43:16PM +0200, Paolo Bonzini wrote: >>> The pvpanic situation is already messed up enough. Let us give our >>> libvirt friends an easy indication that we have untied our side. >>> >>> Not-yet-signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> ... because we first have to determine how to expose the device's existence >>> in the ACPI tables or in fw_cfg. >>> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> >> So it's isa-pvpanic meaning "I-am-sure-this-is-the-last-bug-pvpanic" >> >> If we feel there's need to give libvirt a way to do >> introspection into QEMU bugs, let's architect one. >> Randomly renaming devices in the vain hope it's >> the last major bug is not it. >> >> NACK > > Seconded. While we shouldn't rule out renaming devices, doing so as a > criteria for libvirt sounds utterly wrong. > > Paolo, you are right that a single "NACK" cannot be a criteria, but a > single convincing justification by a random reviewer should be > sufficient to reconsider. :) > > Adding some device property or obtaining the info via some existing > query-* QMP command might be better alternatives. Device properties are a good way to communicate changes to the guest, but here the guest ABI is unchanged. Anyhow, this patch is not strictly necessary. It's just a safety net to avoid that libvirt uses a buggy device on buggy versions of QEMU. Paolo
Il 22/08/2013 14:43, Laszlo Ersek ha scritto: > On 08/21/13 19:06, Paolo Bonzini wrote: >> Il 21/08/2013 19:07, Michael S. Tsirkin ha scritto: > >>> NACK >> >> You know that a single developer's NACK counts nothing (it can be you, >> it can be me), don't you? > > going meta... > > What's this? > > All I know (... I think I know) about patch acceptance is that Anthony > prefers to have at least one R-b. As far as I've seen this is not a hard > requirement (for example, maintainers sometimes send unreviewed patches > in a pull request, and on occasion they are merged). > > No words have been spent on NAKs yet (... since my subscription, that > is). Is this stuff formalized somewhere? > > Sorry for wasting time... No, it's not. But for example I NACKed removal of pvpanic from 1.6, it was overridden, and I didn't complain too much. Paolo
On 08/21/13 19:06, Paolo Bonzini wrote: > Il 21/08/2013 19:07, Michael S. Tsirkin ha scritto: >> NACK > > You know that a single developer's NACK counts nothing (it can be you, > it can be me), don't you? going meta... What's this? All I know (... I think I know) about patch acceptance is that Anthony prefers to have at least one R-b. As far as I've seen this is not a hard requirement (for example, maintainers sometimes send unreviewed patches in a pull request, and on occasion they are merged). No words have been spent on NAKs yet (... since my subscription, that is). Is this stuff formalized somewhere? Sorry for wasting time... Thanks, Laszlo
Laszlo Ersek <lersek@redhat.com> writes: > On 08/21/13 19:06, Paolo Bonzini wrote: >> Il 21/08/2013 19:07, Michael S. Tsirkin ha scritto: > >>> NACK >> >> You know that a single developer's NACK counts nothing (it can be you, >> it can be me), don't you? > > going meta... > > What's this? > > All I know (... I think I know) about patch acceptance is that Anthony > prefers to have at least one R-b. As far as I've seen this is not a hard > requirement (for example, maintainers sometimes send unreviewed patches > in a pull request, and on occasion they are merged). I look very poorly on anyone nacking anything. I value constructive feedback. Nacking does not add any value to the conversation. I admire the fact that we've been able to maintain a very high level of conversation over the years on qemu-devel and throwing around nacks just lowers the overall tone. If you can't think of anything better to say than NACK, don't even bother sending the email in the first place. Regards, Anthony Liguori > > No words have been spent on NAKs yet (... since my subscription, that > is). Is this stuff formalized somewhere? > > Sorry for wasting time... > > Thanks, > Laszlo
On Thu, Aug 22, 2013 at 11:50:43AM -0500, Anthony Liguori wrote: > Laszlo Ersek <lersek@redhat.com> writes: > > > On 08/21/13 19:06, Paolo Bonzini wrote: > >> Il 21/08/2013 19:07, Michael S. Tsirkin ha scritto: > > > >>> NACK > >> > >> You know that a single developer's NACK counts nothing (it can be you, > >> it can be me), don't you? > > > > going meta... > > > > What's this? > > > > All I know (... I think I know) about patch acceptance is that Anthony > > prefers to have at least one R-b. As far as I've seen this is not a hard > > requirement (for example, maintainers sometimes send unreviewed patches > > in a pull request, and on occasion they are merged). > > I look very poorly on anyone nacking anything. I value constructive > feedback. > Nacking does not add any value to the conversation. I admire the fact > that we've been able to maintain a very high level of conversation over > the years on qemu-devel and throwing around nacks just lowers the > overall tone. In that case, what's a good way to clarify that one is opposed to the idea, not the implementation? We have Acked-by: versus Reviewed-by: on the positive side, and I was looking for something like this on the negative side. > > If you can't think of anything better to say than NACK, don't even > bother sending the email in the first place. I did add motivation too, it was snipped in the response. > Regards, > > Anthony Liguori > > > > > No words have been spent on NAKs yet (... since my subscription, that > > is). Is this stuff formalized somewhere? > > > > Sorry for wasting time... > > > > Thanks, > > Laszlo
On Thu, Aug 22, 2013 at 02:41:51PM +0200, Paolo Bonzini wrote: > Il 22/08/2013 14:43, Laszlo Ersek ha scritto: > > On 08/21/13 19:06, Paolo Bonzini wrote: > >> Il 21/08/2013 19:07, Michael S. Tsirkin ha scritto: > > > >>> NACK > >> > >> You know that a single developer's NACK counts nothing (it can be you, > >> it can be me), don't you? > > > > going meta... > > > > What's this? > > > > All I know (... I think I know) about patch acceptance is that Anthony > > prefers to have at least one R-b. As far as I've seen this is not a hard > > requirement (for example, maintainers sometimes send unreviewed patches > > in a pull request, and on occasion they are merged). > > > > No words have been spent on NAKs yet (... since my subscription, that > > is). Is this stuff formalized somewhere? > > > > Sorry for wasting time... > > No, it's not. But for example I NACKed removal of pvpanic from 1.6, it > was overridden, and I didn't complain too much. > > Paolo I don't think it was overridden. In fact you NACKed an explicit -device pvpanic. You suggested disabling in 1.6 but keeping it a builtin, but this was never implemented, afterwards issues with Linux guests surfaced, we discussed this again on the KVM call, and there seemed to be a concensus that it's an OK patch, with some issues. A week later Marcel sent v2, it worked and looked like the least problematic path to take.
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c index 1928cc9..b53011d 100644 --- a/hw/misc/pvpanic.c +++ b/hw/misc/pvpanic.c @@ -27,7 +27,7 @@ /* The pv event value */ #define PVPANIC_PANICKED (1 << PVPANIC_F_PANICKED) -#define TYPE_ISA_PVPANIC_DEVICE "pvpanic" +#define TYPE_ISA_PVPANIC_DEVICE "isa-pvpanic" #define ISA_PVPANIC_DEVICE(obj) \ OBJECT_CHECK(PVPanicState, (obj), TYPE_ISA_PVPANIC_DEVICE)
The pvpanic situation is already messed up enough. Let us give our libvirt friends an easy indication that we have untied our side. Not-yet-signed-off-by: Paolo Bonzini <pbonzini@redhat.com> ... because we first have to determine how to expose the device's existence in the ACPI tables or in fw_cfg. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/misc/pvpanic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)