Re: [Qemu-devel] [PATCH] vhost_net: fix misuse of assert()

[Michael S. Tsirkin]

On Fri, Jun 19, 2015 at 04:00:05PM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > On Fri, Jun 19, 2015 at 12:55:57PM +0200, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <address@hidden> writes:
> >> 
> >> > On Fri, Jun 19, 2015 at 11:13:40AM +0100, Peter Maydell wrote:
> >> >> On 19 June 2015 at 11:07, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
> >> >> >> Signed-off-by: Greg Kurz <address@hidden>
> >> >> >
> >> >> > 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 <address@hidden>
> >> Signed-off-by: Greg Kurz <address@hidden>
> >> 
> >> 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" <address@hidden>
> >
> > 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.