[PATCH] ib-diags: Add cast to fix build on windows

[PATCH] ib-diags: Add cast to fix build on windows

[Hefty, Sean]

Signed-off-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 libibnetdisc/src/ibnetdisc.c |    2 +-
 src/ibportstate.c            |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
 mode change 100644 => 100755 libibnetdisc/src/ibnetdisc.c
 mode change 100644 => 100755 src/ibportstate.c

diff --git a/libibnetdisc/src/ibnetdisc.c b/libibnetdisc/src/ibnetdisc.c
index 86210eb..c93e7ad
--- a/libibnetdisc/src/ibnetdisc.c
+++ b/libibnetdisc/src/ibnetdisc.c
@@ -201,7 +201,7 @@ static void debug_port(ib_portid_t * portid, ibnd_port_t * port)
 
 static int is_mlnx_ext_port_info_supported(ibnd_port_t * port)
 {
-	uint16_t devid = mad_get_field(port->node->info, 0, IB_NODE_DEVID_F);
+	uint16_t devid = (uint16_t) mad_get_field(port->node->info, 0, IB_NODE_DEVID_F);
 
 	if (devid == 0xc738)
 		return 1;
diff --git a/src/ibportstate.c b/src/ibportstate.c
index 81d5b58..daa8514
--- a/src/ibportstate.c
+++ b/src/ibportstate.c
@@ -462,7 +462,7 @@ int main(int argc, char **argv)
 		port_op = QUERY;
 
 	is_switch = get_node_info(&portid, data);
-	devid = mad_get_field(data, 0, IB_NODE_DEVID_F);
+	devid = (uint16_t) mad_get_field(data, 0, IB_NODE_DEVID_F);
 
 	if (port_op != QUERY || changed)
 		printf("Initial %s PortInfo:\n", is_switch ? "Switch" : "CA");
@@ -591,7 +591,7 @@ int main(int argc, char **argv)
 
 			/* Get peer port NodeInfo to obtain peer port number */
 			is_peer_switch = get_node_info(&peerportid, data);
-			rem_devid = mad_get_field(data, 0, IB_NODE_DEVID_F);
+			rem_devid = (uint16_t) mad_get_field(data, 0, IB_NODE_DEVID_F);
 
 			mad_decode_field(data, IB_NODE_LOCAL_PORT_F,
 					 &peerlocalportnum);


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib-diags: Add cast to fix build on windows

[Bart Van Assche]

On Thu, Sep 22, 2011 at 8:29 PM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> Signed-off-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  libibnetdisc/src/ibnetdisc.c |    2 +-
>  src/ibportstate.c            |    4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>  mode change 100644 => 100755 libibnetdisc/src/ibnetdisc.c
>  mode change 100644 => 100755 src/ibportstate.c

A minor nit: is this mode change really necessary ? (I know Visual
Studio does enable the execute bit without asking.)

> diff --git a/libibnetdisc/src/ibnetdisc.c b/libibnetdisc/src/ibnetdisc.c
> index 86210eb..c93e7ad
> --- a/libibnetdisc/src/ibnetdisc.c
> +++ b/libibnetdisc/src/ibnetdisc.c
> @@ -201,7 +201,7 @@ static void debug_port(ib_portid_t * portid, ibnd_port_t * port)
>
>  static int is_mlnx_ext_port_info_supported(ibnd_port_t * port)
>  {
> -       uint16_t devid = mad_get_field(port->node->info, 0, IB_NODE_DEVID_F);
> +       uint16_t devid = (uint16_t) mad_get_field(port->node->info, 0, IB_NODE_DEVID_F);

Has it been considered to disable the MSVC warnings about possible
truncation instead of inserting a cast ? MSVC generates such warnings
while gcc does not.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib-diags: Add cast to fix build on windows

[Hal Rosenstock]

On 9/22/2011 2:36 PM, Bart Van Assche wrote:
>> libibnetdisc/src/ibnetdisc.c |    2 +-
>> >  src/ibportstate.c            |    4 ++--
>> >  2 files changed, 3 insertions(+), 3 deletions(-)
>> >  mode change 100644 => 100755 libibnetdisc/src/ibnetdisc.c
>> >  mode change 100644 => 100755 src/ibportstate.c
> A minor nit: is this mode change really necessary ? (I know Visual
> Studio does enable the execute bit without asking.)

It shouldn't add +w to the permissions of those files in linux.

-- Hal

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] ib-diags: Add cast to fix build on windows

[Hefty, Sean]

> >  libibnetdisc/src/ibnetdisc.c |    2 +-
> >  src/ibportstate.c            |    4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >  mode change 100644 => 100755 libibnetdisc/src/ibnetdisc.c
> >  mode change 100644 => 100755 src/ibportstate.c
> 
> A minor nit: is this mode change really necessary ? (I know Visual
> Studio does enable the execute bit without asking.)

No - these mode changes should not be there.  I removed them further down in the patch, but missed it here.  I modified the files using Eclipse from a Windows system.  Maybe there's some setting that I missed.

Ira, please let me know if you want me to generate a new patch.

> Has it been considered to disable the MSVC warnings about possible
> truncation instead of inserting a cast ? MSVC generates such warnings
> while gcc does not.

The entire windows tree is built using the WinDDK compiler.  I haven't found a way to disable that warning that works.  Since the warning is real, an explicit cast at least lets someone reading the code know that the author took truncation into account.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib-diags: Add cast to fix build on windows

[Jason Gunthorpe]

On Thu, Sep 22, 2011 at 08:20:54PM +0000, Hefty, Sean wrote:
 
> The entire windows tree is built using the WinDDK compiler.  I
> haven't found a way to disable that warning that works.  Since the
> warning is real, an explicit cast at least lets someone reading the
> code know that the author took truncation into account.

It would be much better to fix this at the source.

 mad_get_field16(..,IB_NODE_DEVID_F)

Then at least there is the option of doing a static or runtime check
that IB_NODE_DEVID_F is actually referring to a 16 bit field.

For instance, a static check might be possible with some compilers by
doing:

enum MAD_FIELDS8
{
IB_PORT_LOCAL_PORT_F
};

enum MAD_FIELDS16
{
IB_NODE_DEVID_F
};

uint8_t mad_get_field8(void *buf, int base_offs,
	               enum MAD_FIELDS8 field);
uint16_t mad_get_field16(void *buf, int base_offs,
	                 enum MAD_FIELDS16 field);

A compiler can produce a warning about mismatching enums.

Someone can fixup the enums later, but I'd much rather see you add a
patch with this hunk:

static inline uint16_t mad_get_field16(void *buf, int base_offs,
	                               enum MAD_FIELDS field)
{
   return (uint16_t)mad_get_field(buf,base_offs,field);
}

And another patch replacing every (uint16_t)mad_get_field with
mad_get_field16

Adding a cast is just pointless noise, doesn't fix or prove anything.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] ib-diags: Add cast to fix build on windows

[Hefty, Sean]

> Adding a cast is just pointless noise, doesn't fix or prove anything.

It fixes the build on windows.  You can call that pointless, but I do not.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib-diags: Add cast to fix build on windows

[Jason Gunthorpe]

On Thu, Sep 22, 2011 at 09:33:24PM +0000, Hefty, Sean wrote:
> > Adding a cast is just pointless noise, doesn't fix or prove anything.
> 
> It fixes the build on windows.  You can call that pointless, but I do not.

It supresses a warning, warnings are not build failures.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] ib-diags: Add cast to fix build on windows

[Hefty, Sean]

> It supresses a warning, warnings are not build failures.

Actually, on windows the build fails.  No executables are built.

mad_[get|set]_field[8|16|32] should work fine.  But those are exported from ibmad, which would require ib-diags to either check for them as a requirement or implement them if not found.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib-diags: Add cast to fix build on windows

[Jason Gunthorpe]

On Thu, Sep 22, 2011 at 11:03:24PM +0000, Hefty, Sean wrote:
> > It supresses a warning, warnings are not build failures.
> 
> Actually, on windows the build fails.  No executables are built.

Seriously? I hope this is because you've got the warnings are errors
flag set..

> mad_[get|set]_field[8|16|32] should work fine.  But those are
> exported from ibmad, which would require ib-diags to either check
> for them as a requirement or implement them if not found.

Everyone using ibmad on windows should have this problem, so fixing
ibmad seems proper to me.. Tag the no-size ones with deprecated and
you will no longer have this problem in your Windows porting efforts
because all the Linux compilers will complain.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html