| Message ID | 20220329093545.52114-1-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-7.0] main-loop: Disable GLOBAL_STATE_CODE() assertions | expand |
On 29/3/22 11:35, Hanna Reitz wrote: > These assertions are very useful for developers to find bugs, and so > they have indeed pointed us towards bugs already. For users, it is not > so useful to find these bugs. We should probably not enable them in > releases until we are sufficiently certain that they will not fire > during normal operation, unless something is going seriously wrong. > > For example, we have received a bug report that you cannot add an NBD > server on a BDS in an I/O thread with `-incoming defer`. I am sure this > is a real bug that needs investigation, but we do not really have that > time right now, so close to release, and so I would rather disable the > assertions to get time to investigate such reports. > > (I am just putting the link as "buglink" below, not "closes", because > disabling the assertion will not fix the likely underlying bug.) > > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/945 Also helps: https://gitlab.com/qemu-project/qemu/-/issues/926 > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > include/qemu/main-loop.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h > index 7a4d6a0920..3bf8aeb3cd 100644 > --- a/include/qemu/main-loop.h > +++ b/include/qemu/main-loop.h > @@ -272,7 +272,8 @@ bool qemu_in_main_thread(void); > /* Mark and check that the function is part of the global state API. */ > #define GLOBAL_STATE_CODE() \ > do { \ > - assert(qemu_in_main_thread()); \ > + /* FIXME: Re-enable after 7.0 release */ \ > + /* assert(qemu_in_main_thread()); */ \ > } while (0) > > /* Mark and check that the function is part of the I/O API. */ Do you want me to cancel/repost my PR without this patch? https://lore.kernel.org/qemu-devel/20220328224012.32737-3-philippe.mathieu.daude@gmail.com/ Otherwise: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 29.03.22 12:17, Philippe Mathieu-Daudé wrote: > On 29/3/22 11:35, Hanna Reitz wrote: >> These assertions are very useful for developers to find bugs, and so >> they have indeed pointed us towards bugs already. For users, it is not >> so useful to find these bugs. We should probably not enable them in >> releases until we are sufficiently certain that they will not fire >> during normal operation, unless something is going seriously wrong. >> >> For example, we have received a bug report that you cannot add an NBD >> server on a BDS in an I/O thread with `-incoming defer`. I am sure this >> is a real bug that needs investigation, but we do not really have that >> time right now, so close to release, and so I would rather disable the >> assertions to get time to investigate such reports. >> >> (I am just putting the link as "buglink" below, not "closes", because >> disabling the assertion will not fix the likely underlying bug.) >> >> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/945 > > Also helps: > https://gitlab.com/qemu-project/qemu/-/issues/926 > >> Signed-off-by: Hanna Reitz <hreitz@redhat.com> >> --- >> include/qemu/main-loop.h | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h >> index 7a4d6a0920..3bf8aeb3cd 100644 >> --- a/include/qemu/main-loop.h >> +++ b/include/qemu/main-loop.h >> @@ -272,7 +272,8 @@ bool qemu_in_main_thread(void); >> /* Mark and check that the function is part of the global state >> API. */ >> #define GLOBAL_STATE_CODE() \ >> do { \ >> - assert(qemu_in_main_thread()); \ >> + /* FIXME: Re-enable after 7.0 release */ \ >> + /* assert(qemu_in_main_thread()); */ \ >> } while (0) >> /* Mark and check that the function is part of the I/O API. */ > > Do you want me to cancel/repost my PR without this patch? > > https://lore.kernel.org/qemu-devel/20220328224012.32737-3-philippe.mathieu.daude@gmail.com/ I think we should let Peter take your PR first, as long as the discussion on this is still out. I’d like to give it a couple more hours, and as far as I understand, we definitely want yours. (Taking mine will then cause me rebase conflicts and make it look weird, but that’s not too bad for something that’s just a temporary band-aid anyway.) Hanna
Am 29/03/2022 um 11:35 schrieb Hanna Reitz: > These assertions are very useful for developers to find bugs, and so > they have indeed pointed us towards bugs already. For users, it is not > so useful to find these bugs. We should probably not enable them in > releases until we are sufficiently certain that they will not fire > during normal operation, unless something is going seriously wrong. > > For example, we have received a bug report that you cannot add an NBD > server on a BDS in an I/O thread with `-incoming defer`. I am sure this > is a real bug that needs investigation, but we do not really have that > time right now, so close to release, and so I would rather disable the > assertions to get time to investigate such reports. > > (I am just putting the link as "buglink" below, not "closes", because > disabling the assertion will not fix the likely underlying bug.) > > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/945 > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > include/qemu/main-loop.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h > index 7a4d6a0920..3bf8aeb3cd 100644 > --- a/include/qemu/main-loop.h > +++ b/include/qemu/main-loop.h > @@ -272,7 +272,8 @@ bool qemu_in_main_thread(void); > /* Mark and check that the function is part of the global state API. */ > #define GLOBAL_STATE_CODE() \ > do { \ > - assert(qemu_in_main_thread()); \ > + /* FIXME: Re-enable after 7.0 release */ \ > + /* assert(qemu_in_main_thread()); */ \ > } while (0) > > /* Mark and check that the function is part of the I/O API. */ > Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
On Tue, Mar 29, 2022 at 11:35:45AM +0200, Hanna Reitz wrote: > These assertions are very useful for developers to find bugs, and so > they have indeed pointed us towards bugs already. For users, it is not > so useful to find these bugs. We should probably not enable them in > releases until we are sufficiently certain that they will not fire > during normal operation, unless something is going seriously wrong. > > For example, we have received a bug report that you cannot add an NBD > server on a BDS in an I/O thread with `-incoming defer`. I am sure this > is a real bug that needs investigation, but we do not really have that > time right now, so close to release, and so I would rather disable the > assertions to get time to investigate such reports. > > (I am just putting the link as "buglink" below, not "closes", because > disabling the assertion will not fix the likely underlying bug.) > > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/945 > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > include/qemu/main-loop.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Sounds reasonable to me. Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index 7a4d6a0920..3bf8aeb3cd 100644 --- a/include/qemu/main-loop.h +++ b/include/qemu/main-loop.h @@ -272,7 +272,8 @@ bool qemu_in_main_thread(void); /* Mark and check that the function is part of the global state API. */ #define GLOBAL_STATE_CODE() \ do { \ - assert(qemu_in_main_thread()); \ + /* FIXME: Re-enable after 7.0 release */ \ + /* assert(qemu_in_main_thread()); */ \ } while (0) /* Mark and check that the function is part of the I/O API. */
These assertions are very useful for developers to find bugs, and so they have indeed pointed us towards bugs already. For users, it is not so useful to find these bugs. We should probably not enable them in releases until we are sufficiently certain that they will not fire during normal operation, unless something is going seriously wrong. For example, we have received a bug report that you cannot add an NBD server on a BDS in an I/O thread with `-incoming defer`. I am sure this is a real bug that needs investigation, but we do not really have that time right now, so close to release, and so I would rather disable the assertions to get time to investigate such reports. (I am just putting the link as "buglink" below, not "closes", because disabling the assertion will not fix the likely underlying bug.) Buglink: https://gitlab.com/qemu-project/qemu/-/issues/945 Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- include/qemu/main-loop.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)