* [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()
@ 2015-06-19 10:00 Greg Kurz
2015-06-19 10:07 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Greg Kurz @ 2015-06-19 10:00 UTC (permalink / raw
To: Michael S. Tsirkin; +Cc: qemu-devel
In case NDEBUG is defined, assert() expands to nothing and
vhost_net_set_vnet_endian() doesn't get called...
Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/net/vhost_net.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Michael,
Yet another fix for the cross-endian vhost series... please
apply.
--
Greg
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f505c91d420a..6d3b7879f8ef 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -376,7 +376,8 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
}
assert(r >= 0);
- assert(vhost_net_set_vnet_endian(dev, ncs[0].peer, false) >= 0);
+ r = vhost_net_set_vnet_endian(dev, ncs[0].peer, false);
+ assert(r >= 0);
}
void vhost_net_cleanup(struct vhost_net *net)
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()
2015-06-19 10:00 [Qemu-devel] [PATCH] vhost_net: fix misuse of assert() Greg Kurz
@ 2015-06-19 10:07 ` Michael S. Tsirkin
2015-06-19 10:13 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2015-06-19 10:07 UTC (permalink / raw
To: Greg Kurz; +Cc: qemu-devel
On Fri, Jun 19, 2015 at 12:00:53PM +0200, Greg Kurz wrote:
> In case NDEBUG is defined, assert() expands to nothing and
> vhost_net_set_vnet_endian() doesn't get called...
>
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Not sure what the point is.
We don't support building with NDEBUG.
> ---
> hw/net/vhost_net.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Michael,
>
> Yet another fix for the cross-endian vhost series... please
> apply.
>
> --
> Greg
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index f505c91d420a..6d3b7879f8ef 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -376,7 +376,8 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
> }
> assert(r >= 0);
>
> - assert(vhost_net_set_vnet_endian(dev, ncs[0].peer, false) >= 0);
> + r = vhost_net_set_vnet_endian(dev, ncs[0].peer, false);
> + assert(r >= 0);
> }
>
> void vhost_net_cleanup(struct vhost_net *net)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()
2015-06-19 10:07 ` Michael S. Tsirkin
@ 2015-06-19 10:13 ` Peter Maydell
2015-06-19 10:15 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2015-06-19 10:13 UTC (permalink / raw
To: Michael S. Tsirkin; +Cc: QEMU Developers
On 19 June 2015 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Jun 19, 2015 at 12:00:53PM +0200, Greg Kurz wrote:
>> In case NDEBUG is defined, assert() expands to nothing and
>> vhost_net_set_vnet_endian() doesn't get called...
>>
>> Suggested-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>
> Not sure what the point is.
> We don't support building with NDEBUG.
Putting functional behaviour inside an assert() is still a really
bad idea. If you're reading the code you probably skim over the
assert() as not functionally relevant...
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()
2015-06-19 10:13 ` Peter Maydell
@ 2015-06-19 10:15 ` Michael S. Tsirkin
2015-06-19 10:55 ` Markus Armbruster
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2015-06-19 10:15 UTC (permalink / raw
To: Peter Maydell; +Cc: QEMU Developers
On Fri, Jun 19, 2015 at 11:13:40AM +0100, Peter Maydell wrote:
> On 19 June 2015 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Jun 19, 2015 at 12:00:53PM +0200, Greg Kurz wrote:
> >> In case NDEBUG is defined, assert() expands to nothing and
> >> vhost_net_set_vnet_endian() doesn't get called...
> >>
> >> Suggested-by: Thomas Huth <thuth@redhat.com>
> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >
> > Not sure what the point is.
> > We don't support building with NDEBUG.
>
> Putting functional behaviour inside an assert() is still a really
> bad idea. If you're reading the code you probably skim over the
> assert() as not functionally relevant...
>
> -- PMM
I can apply this if commit log explains it's a readability
enhancement, not a bugfix.
--
MST
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()
2015-06-19 10:15 ` Michael S. Tsirkin
@ 2015-06-19 10:55 ` Markus Armbruster
2015-06-19 13:37 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2015-06-19 10:55 UTC (permalink / raw
To: Michael S. Tsirkin; +Cc: Peter Maydell, QEMU Developers
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Jun 19, 2015 at 11:13:40AM +0100, Peter Maydell wrote:
>> On 19 June 2015 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Jun 19, 2015 at 12:00:53PM +0200, Greg Kurz wrote:
>> >> In case NDEBUG is defined, assert() expands to nothing and
>> >> vhost_net_set_vnet_endian() doesn't get called...
>> >>
>> >> Suggested-by: Thomas Huth <thuth@redhat.com>
>> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> >
>> > Not sure what the point is.
>> > We don't support building with NDEBUG.
>>
>> Putting functional behaviour inside an assert() is still a really
>> bad idea. If you're reading the code you probably skim over the
>> assert() as not functionally relevant...
>>
>> -- PMM
>
> I can apply this if commit log explains it's a readability
> enhancement, not a bugfix.
Easy:
vhost_net: fix misuse of assert()
In case NDEBUG is defined, assert() expands to nothing and
vhost_net_set_vnet_endian() doesn't get called...
Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
We don't support building with NDEBUG, but putting functional behaviour
inside an assert() is still a really bad idea. If you're reading the
code you probably skim over the assert() as not functionally relevant...
Signed-off-by: Michael S. Tsirkin" <mst@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()
2015-06-19 10:55 ` Markus Armbruster
@ 2015-06-19 13:37 ` Michael S. Tsirkin
2015-06-19 14:00 ` Markus Armbruster
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2015-06-19 13:37 UTC (permalink / raw
To: Markus Armbruster; +Cc: Peter Maydell, QEMU Developers
On Fri, Jun 19, 2015 at 12:55:57PM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> > On Fri, Jun 19, 2015 at 11:13:40AM +0100, Peter Maydell wrote:
> >> On 19 June 2015 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Fri, Jun 19, 2015 at 12:00:53PM +0200, Greg Kurz wrote:
> >> >> In case NDEBUG is defined, assert() expands to nothing and
> >> >> vhost_net_set_vnet_endian() doesn't get called...
> >> >>
> >> >> Suggested-by: Thomas Huth <thuth@redhat.com>
> >> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >> >
> >> > Not sure what the point is.
> >> > We don't support building with NDEBUG.
> >>
> >> Putting functional behaviour inside an assert() is still a really
> >> bad idea. If you're reading the code you probably skim over the
> >> assert() as not functionally relevant...
> >>
> >> -- PMM
> >
> > I can apply this if commit log explains it's a readability
> > enhancement, not a bugfix.
>
> Easy:
>
> vhost_net: fix misuse of assert()
>
> In case NDEBUG is defined, assert() expands to nothing and
> vhost_net_set_vnet_endian() doesn't get called...
>
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>
> We don't support building with NDEBUG, but putting functional behaviour
> inside an assert() is still a really bad idea. If you're reading the
> code you probably skim over the assert() as not functionally relevant...
>
> Signed-off-by: Michael S. Tsirkin" <mst@redhat.com>
Pls submit v2 in the regular format - I can rewrite the commit log
but prefer not to, as it doesn't scale. There's no need to add my
signature though, git am -s does this automatically.
Thanks!
--
MST
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()
2015-06-19 13:37 ` Michael S. Tsirkin
@ 2015-06-19 14:00 ` Markus Armbruster
2015-06-19 14:06 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2015-06-19 14:00 UTC (permalink / raw
To: Michael S. Tsirkin; +Cc: Peter Maydell, QEMU Developers
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Jun 19, 2015 at 12:55:57PM +0200, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>> > On Fri, Jun 19, 2015 at 11:13:40AM +0100, Peter Maydell wrote:
>> >> On 19 June 2015 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> > On Fri, Jun 19, 2015 at 12:00:53PM +0200, Greg Kurz wrote:
>> >> >> In case NDEBUG is defined, assert() expands to nothing and
>> >> >> vhost_net_set_vnet_endian() doesn't get called...
>> >> >>
>> >> >> Suggested-by: Thomas Huth <thuth@redhat.com>
>> >> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> >> >
>> >> > Not sure what the point is.
>> >> > We don't support building with NDEBUG.
>> >>
>> >> Putting functional behaviour inside an assert() is still a really
>> >> bad idea. If you're reading the code you probably skim over the
>> >> assert() as not functionally relevant...
>> >>
>> >> -- PMM
>> >
>> > I can apply this if commit log explains it's a readability
>> > enhancement, not a bugfix.
>>
>> Easy:
>>
>> vhost_net: fix misuse of assert()
>>
>> In case NDEBUG is defined, assert() expands to nothing and
>> vhost_net_set_vnet_endian() doesn't get called...
>>
>> Suggested-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>
>> We don't support building with NDEBUG, but putting functional behaviour
>> inside an assert() is still a really bad idea. If you're reading the
>> code you probably skim over the assert() as not functionally relevant...
>>
>> Signed-off-by: Michael S. Tsirkin" <mst@redhat.com>
>
> Pls submit v2 in the regular format - I can rewrite the commit log
> but prefer not to, as it doesn't scale. There's no need to add my
> signature though, git am -s does this automatically.
I *hate* it when maintainers rewrite my commit message, then claim it's
mine :)
But that's not what I suggested! I suggested to append the additional
explanation you want after Greg's S-o-B. Makes it obvious that it's
yours.
I think that scales just fine in a simple case like this where you
already know the explanation.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()
2015-06-19 14:00 ` Markus Armbruster
@ 2015-06-19 14:06 ` Michael S. Tsirkin
2015-06-19 14:37 ` Greg Kurz
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2015-06-19 14:06 UTC (permalink / raw
To: Markus Armbruster; +Cc: Peter Maydell, QEMU Developers
On Fri, Jun 19, 2015 at 04:00:05PM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> > On Fri, Jun 19, 2015 at 12:55:57PM +0200, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >>
> >> > On Fri, Jun 19, 2015 at 11:13:40AM +0100, Peter Maydell wrote:
> >> >> On 19 June 2015 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> > On Fri, Jun 19, 2015 at 12:00:53PM +0200, Greg Kurz wrote:
> >> >> >> In case NDEBUG is defined, assert() expands to nothing and
> >> >> >> vhost_net_set_vnet_endian() doesn't get called...
> >> >> >>
> >> >> >> Suggested-by: Thomas Huth <thuth@redhat.com>
> >> >> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >> >> >
> >> >> > Not sure what the point is.
> >> >> > We don't support building with NDEBUG.
> >> >>
> >> >> Putting functional behaviour inside an assert() is still a really
> >> >> bad idea. If you're reading the code you probably skim over the
> >> >> assert() as not functionally relevant...
> >> >>
> >> >> -- PMM
> >> >
> >> > I can apply this if commit log explains it's a readability
> >> > enhancement, not a bugfix.
> >>
> >> Easy:
> >>
> >> vhost_net: fix misuse of assert()
> >>
> >> In case NDEBUG is defined, assert() expands to nothing and
> >> vhost_net_set_vnet_endian() doesn't get called...
> >>
> >> Suggested-by: Thomas Huth <thuth@redhat.com>
> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >>
> >> We don't support building with NDEBUG, but putting functional behaviour
> >> inside an assert() is still a really bad idea. If you're reading the
> >> code you probably skim over the assert() as not functionally relevant...
> >>
> >> Signed-off-by: Michael S. Tsirkin" <mst@redhat.com>
> >
> > Pls submit v2 in the regular format - I can rewrite the commit log
> > but prefer not to, as it doesn't scale. There's no need to add my
> > signature though, git am -s does this automatically.
>
> I *hate* it when maintainers rewrite my commit message, then claim it's
> mine :)
>
> But that's not what I suggested! I suggested to append the additional
> explanation you want after Greg's S-o-B. Makes it obvious that it's
> yours.
Well both the subject and the original commit log are irrelevant IMO:
why mention NDEBUG when we don't support it? So it's not really
a misuse.
> I think that scales just fine in a simple case like this where you
> already know the explanation.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()
2015-06-19 14:06 ` Michael S. Tsirkin
@ 2015-06-19 14:37 ` Greg Kurz
0 siblings, 0 replies; 9+ messages in thread
From: Greg Kurz @ 2015-06-19 14:37 UTC (permalink / raw
To: Michael S. Tsirkin; +Cc: Peter Maydell, Markus Armbruster, QEMU Developers
On Fri, 19 Jun 2015 16:06:04 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Jun 19, 2015 at 04:00:05PM +0200, Markus Armbruster wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >
> > > On Fri, Jun 19, 2015 at 12:55:57PM +0200, Markus Armbruster wrote:
> > >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > >>
> > >> > On Fri, Jun 19, 2015 at 11:13:40AM +0100, Peter Maydell wrote:
> > >> >> On 19 June 2015 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >> >> > On Fri, Jun 19, 2015 at 12:00:53PM +0200, Greg Kurz wrote:
> > >> >> >> In case NDEBUG is defined, assert() expands to nothing and
> > >> >> >> vhost_net_set_vnet_endian() doesn't get called...
> > >> >> >>
> > >> >> >> Suggested-by: Thomas Huth <thuth@redhat.com>
> > >> >> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > >> >> >
> > >> >> > Not sure what the point is.
> > >> >> > We don't support building with NDEBUG.
> > >> >>
> > >> >> Putting functional behaviour inside an assert() is still a really
> > >> >> bad idea. If you're reading the code you probably skim over the
> > >> >> assert() as not functionally relevant...
> > >> >>
> > >> >> -- PMM
> > >> >
> > >> > I can apply this if commit log explains it's a readability
> > >> > enhancement, not a bugfix.
> > >>
> > >> Easy:
> > >>
> > >> vhost_net: fix misuse of assert()
> > >>
> > >> In case NDEBUG is defined, assert() expands to nothing and
> > >> vhost_net_set_vnet_endian() doesn't get called...
> > >>
> > >> Suggested-by: Thomas Huth <thuth@redhat.com>
> > >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > >>
> > >> We don't support building with NDEBUG, but putting functional behaviour
> > >> inside an assert() is still a really bad idea. If you're reading the
> > >> code you probably skim over the assert() as not functionally relevant...
> > >>
> > >> Signed-off-by: Michael S. Tsirkin" <mst@redhat.com>
> > >
> > > Pls submit v2 in the regular format - I can rewrite the commit log
> > > but prefer not to, as it doesn't scale. There's no need to add my
> > > signature though, git am -s does this automatically.
> >
> > I *hate* it when maintainers rewrite my commit message, then claim it's
> > mine :)
> >
> > But that's not what I suggested! I suggested to append the additional
> > explanation you want after Greg's S-o-B. Makes it obvious that it's
> > yours.
>
> Well both the subject and the original commit log are irrelevant IMO:
> why mention NDEBUG when we don't support it? So it's not really
> a misuse.
>
Hmmm... I got obviously un-Cc'd from this thread at some point... but
anyway I've sent a v2 with a more comprehensive log. The subject stays
the same though: putting relevant code in assert() is wrong, even from
a code reading POV.
> > I think that scales just fine in a simple case like this where you
> > already know the explanation.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-06-19 14:37 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-19 10:00 [Qemu-devel] [PATCH] vhost_net: fix misuse of assert() Greg Kurz
2015-06-19 10:07 ` Michael S. Tsirkin
2015-06-19 10:13 ` Peter Maydell
2015-06-19 10:15 ` Michael S. Tsirkin
2015-06-19 10:55 ` Markus Armbruster
2015-06-19 13:37 ` Michael S. Tsirkin
2015-06-19 14:00 ` Markus Armbruster
2015-06-19 14:06 ` Michael S. Tsirkin
2015-06-19 14:37 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.