Re: [Openvpn-devel] coverity issue 2 - dead default / code cleanup in mtcp.c

2012-12-21 Thread Gert Doering 
Hi,

On Thu, Dec 20, 2012 at 10:10:40AM +0100, David Sommerseth wrote:
> > Cleanup in 2.4?
> 
> Does it hurt to have this "dead code"?  Agreed, if all the flags are
> covered, we'll never see 'default' in action.  But is it possible that
> these flags can be extended at some point in the future, which we need
> to account for?  I somehow find confidence in having this "trap" with
> msg(M_FATAL,...) for future scenarios.  Not because I strongly believe
> it will be needed, but more like a precaution (as in "Yes, we have
> thought about this")

Well, I'd completely get rid of the switch/case here - we only have 
4 different cases, 2 of which are handled in the same subcase, so
"setting up flags and then switch(flags), plus default: handler" seems 
to add more confusion than just handling possible cases directly in
the if() statements...

In general, having a "must not happen" handler to validate assumptions
is a good thing, but if those preconditions are set by code 5 lines up,
it's a bit over the top...

gert

-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgpRgl_QCixTr.pgp
Description: PGP signature

Re: [Openvpn-devel] coverity issue 2 - dead default / code cleanup in mtcp.c

2012-12-20 Thread David Sommerseth 
On 14/12/12 19:18, Gert Doering wrote:
> Hi,
> 
> next one: mtcp.c, about line 470:
> 
>   unsigned int flags = MTP_NONE;
> 
>   if (TUN_OUT(c))
> flags |= MTP_TUN_OUT;
>   if (LINK_OUT(c))
> flags |= MTP_LINK_OUT;
> 
>   switch (flags)
> {
> case MTP_TUN_OUT|MTP_LINK_OUT:
> case MTP_TUN_OUT:
>   newaction = TA_TUN_WRITE;
>   break;
> case MTP_LINK_OUT:
>   newaction = TA_SOCKET_WRITE;
>   break;
> case MTP_NONE:
>   if (mi && socket_read_residual (c->c2.link_socket))
> newaction = TA_SOCKET_READ_RESIDUAL;
>   else
> multi_tcp_set_global_rw_flags (m, mi);
>   break;
> default:
>   {
> struct gc_arena gc = gc_new ();
> msg (M_FATAL, "MULTI TCP: multi_tcp_post bad state, mi=%s flags=%d",
>  multi_instance_string (mi, false, ),
>  flags);
> gc_free ();
> break;
>   }
> }
> 
> 
> 
> coverity is complaining that the "default:" clause can never be reached,
> as all the possible values for "flags" are covered - and it's obviously
> right.  It's not something critical, more a "code cleanup" thing.
> 
> (The whole usage of switch/case here reeks a bit, given that I think the 
> code would be simpler by directly setting "newaction" depending on
> "if (TUN_OUT(c))" etc., not bothering with "flags" in the first place)
> 
> Cleanup in 2.4?

Does it hurt to have this "dead code"?  Agreed, if all the flags are
covered, we'll never see 'default' in action.  But is it possible that
these flags can be extended at some point in the future, which we need
to account for?  I somehow find confidence in having this "trap" with
msg(M_FATAL,...) for future scenarios.  Not because I strongly believe
it will be needed, but more like a precaution (as in "Yes, we have
thought about this")


-- 
kind regards,

David Sommerseth



signature.asc
Description: OpenPGP digital signature

[Openvpn-devel] coverity issue 2 - dead default / code cleanup in mtcp.c

2012-12-14 Thread Gert Doering 
Hi,

next one: mtcp.c, about line 470:

  unsigned int flags = MTP_NONE;

  if (TUN_OUT(c))
flags |= MTP_TUN_OUT;
  if (LINK_OUT(c))
flags |= MTP_LINK_OUT;

  switch (flags)
{
case MTP_TUN_OUT|MTP_LINK_OUT:
case MTP_TUN_OUT:
  newaction = TA_TUN_WRITE;
  break;
case MTP_LINK_OUT:
  newaction = TA_SOCKET_WRITE;
  break;
case MTP_NONE:
  if (mi && socket_read_residual (c->c2.link_socket))
newaction = TA_SOCKET_READ_RESIDUAL;
  else
multi_tcp_set_global_rw_flags (m, mi);
  break;
default:
  {
struct gc_arena gc = gc_new ();
msg (M_FATAL, "MULTI TCP: multi_tcp_post bad state, mi=%s flags=%d",
 multi_instance_string (mi, false, ),
 flags);
gc_free ();
break;
  }
}



coverity is complaining that the "default:" clause can never be reached,
as all the possible values for "flags" are covered - and it's obviously
right.  It's not something critical, more a "code cleanup" thing.

(The whole usage of switch/case here reeks a bit, given that I think the 
code would be simpler by directly setting "newaction" depending on
"if (TUN_OUT(c))" etc., not bothering with "flags" in the first place)

Cleanup in 2.4?

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgpH1RfgWrXJS.pgp
Description: PGP signature