diff mbox

vhost_net: fix misuse of assert()

Message ID 20150619095853.12531.15450.stgit@bahia.huguette.org
State New
Headers show

Commit Message

Greg Kurz June 19, 2015, 10 a.m. UTC 
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

Comments

Michael S. Tsirkin June 19, 2015, 10:07 a.m. UTC | #1 
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)
Peter Maydell June 19, 2015, 10:13 a.m. UTC | #2 
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
Michael S. Tsirkin June 19, 2015, 10:15 a.m. UTC | #3 
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.
Markus Armbruster June 19, 2015, 10:55 a.m. UTC | #4 
"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>
Michael S. Tsirkin June 19, 2015, 1:37 p.m. UTC | #5 
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!
Markus Armbruster June 19, 2015, 2 p.m. UTC | #6 
"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.
Michael S. Tsirkin June 19, 2015, 2:06 p.m. UTC | #7 
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.
Greg Kurz June 19, 2015, 2:37 p.m. UTC | #8 
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.
>
diff mbox

Patch

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)