On 10/25/2012 07:58 PM, Neil Horman wrote:

On Thu, Oct 25, 2012 at 05:42:15PM -0400, Vlad Yasevich wrote:

quoted

On 10/25/2012 04:47 PM, Neil Horman wrote:

quoted

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
  include/net/sctp/command.h  |  7 ++++---
  include/net/sctp/ulpqueue.h |  2 +-
  net/sctp/sm_sideeffect.c    | 45 ++++++++++++++++++++++-----------------------
  net/sctp/ulpqueue.c         |  3 +--
  4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..7f1b0f3 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h

@@ -131,7 +131,6 @@ typedef union {
  	sctp_state_t state;
  	sctp_event_timeout_t to;
  	unsigned long zero;
-	void *ptr;
  	struct sctp_chunk *chunk;
  	struct sctp_association *asoc;
  	struct sctp_transport *transport;

@@ -154,9 +153,12 @@ typedef union {
   * which takes an __s32 and returns a sctp_arg_t containing the
   * __s32.  So, after foo = SCTP_I32(arg), foo.i32 == arg.
   */
+#define SCTP_NULL_BYTE 0xAA
  static inline sctp_arg_t SCTP_NULL(void)
  {
-	sctp_arg_t retval; retval.ptr = NULL; return retval;
+	sctp_arg_t retval;
+	memset(&retval, SCTP_NULL_BYTE, sizeof(sctp_arg_t));
+	return retval;

What's this for?  Can't we just use retval.zero?

-vlad

My intent was to highlight any users of sctp_arg_t when SCTP_NULL was passed.
My thinking was that the 0xAA byte patern would be a good indicator.  Although,
admittedly I didn't see the zero argument there.  Looking at it though, the zero
member of the union is effectively unused.  Strictly speaking its used for
initalization of sctp_arg_t, but its done somewhat poorly, since theres no
guarantee that an unsigned long will be the largest member of that union.  Doing
the memset guarantees the whole instance is set to a predefined value.

I could go either way with this, would you rather we just have SCTP_NULL return
retval = { .zero = 0}; or would you rather remove the zero initialization from
SCTP_[NO]FORCE, and SCTP_ARG_CONSTRUCTOR and do the memset.  I think the memset
reduces to a single 64 bit assignment as long as the union doesn't exceed that
size anyway, and it ensures that you initalize the whole union's storage if it
does in the future.  And if we remove the initialization step (I don't see that
its needed in the three macros above anyway), then we can remove the zero member
as well.

You need the initialization step, otherwise things might fail (they did 
on IA64 a while back).  That's why the zero member was added.  You can 
go with memset if you want, but I was primarily wondering why the 0xAA 
pattern was there.

-vlad

Let me know what you want to do here, and I can respin this.
Best
Neil