Bug 94777

Summary:	test_dhcp incorrect for big endians
Product:	ModemManager	Reporter:	Dan Horák <dan>
Component:	plugins	Assignee:	ModemManager bug user <modemmanager>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	medium	CC:	than
Version:	git master
Hardware:	PowerPC
OS:	All
URL:	https://bugzilla.redhat.com/show_bug.cgi?id=1322696
Whiteboard:
i915 platform:		i915 features:
Attachments:	fix the bigendian issue on ppc64/s390

Description Dan Horák 2016-04-01 14:11:50 UTC

test_dhcp fails on ppc64 and s390(x) (and will also fail on other big endian platforms) with

...
TEST: test-modem-helpers-huawei... lt-ModemManager[42769]: <info>  [1459406354.696341] [main.c:218] main(): ModemManager is shut down
(pid=42792)
  /MM/huawei/ndisstatqry:                                              OK
  /MM/huawei/dhcp:                                                     **
ERROR:huawei/tests/test-modem-helpers-huawei.c:183:test_dhcp: assertion failed (inet_ntoa (*((struct in_addr *) &addr)) == dhcp_tests[i].expected_addr): ("163.236.92.100" == "100.92.236.163")
FAIL
GTester: last random seed: R02S174407a4c52d92398f7aa23fa6555127
Makefile:3478: recipe for target 'test-nonrecursive' failed
make[2]: Leaving directory '/builddir/build/BUILD/ModemManager-1.5.990/plugins'
Makefile:3265: recipe for target 'check-am' failed
make[1]: Leaving directory '/builddir/build/BUILD/ModemManager-1.5.990/plugins'
/bin/sh: line 1: 42704 Terminated              G_DEBUG=gc-friendly MALLOC_CHECK_=2 MALLOC_PERTURB_=$((${RANDOM:-256} % 256)) gtester --verbose test-modem-helpers-icera test-modem-helpers-mbm test-service-generic test-modem-helpers-huawei test-modem-helpers-cinterion test-modem-helpers-thuraya test-modem-helpers-altair-lte test-modem-helpers-telit
make[2]: *** [test-nonrecursive] Error 143
make[1]: *** [check-am] Error 2

for full logs please see http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=2173979 or http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=3268272

for downstream bug see https://bugzilla.redhat.com/show_bug.cgi?id=1322696

Comment 1 than 2016-04-07 15:30:18 UTC

Created attachment 122790 [details] [review]
fix the bigendian issue on ppc64/s390

Comment 2 Aleksander Morgado 2016-04-07 18:42:08 UTC

Comment on attachment 122790 [details] [review]
fix the bigendian issue on ppc64/s390

I was assuming that just some GUINT32_FROM_BE() or GUINT32_FROM_LE() would be enough; those macros already convert from BE/LE to host endian the integer values. Could you check if that approach is easier?

Comment 3 than 2016-04-12 12:29:12 UTC

attach a better and correct fix for endian issue (s390x/ppc64)

diff -up ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c.than ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c
--- ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c.than  2016-04-12 14:03:15.519328232 +0200
+++ ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c       2016-04-12 13:56:58.978491128 +0200
@@ -190,7 +190,11 @@ match_info_to_ip4_addr (GMatchInfo *matc
     if (!bin || bin_len != 4)
         goto done;
 
-    *out_addr = GUINT32_TO_BE (*((guint32 *) bin));
+#if __BYTE_ORDER == __BIG_ENDIAN
+    *out_addr = GUINT32_TO_LE (*((guint32 *) bin));
+#else
+    *out_addr = GUINT32_TO_BE (*((guint32 *) bin));   
+#endif
     success = TRUE;
 
 done:

Comment 4 than 2016-04-12 12:31:31 UTC

it builds fine with the new patch on all fedora platform:

http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=3296203
http://koji.fedoraproject.org/koji/taskinfo?taskID=13634469

Comment 5 Aleksander Morgado 2016-04-12 15:08:10 UTC

(In reply to than from comment #3)
> attach a better and correct fix for endian issue (s390x/ppc64)
> 
> diff -up ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c.than
> ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c
> --- ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c.than 
> 2016-04-12 14:03:15.519328232 +0200
> +++ ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c      
> 2016-04-12 13:56:58.978491128 +0200
> @@ -190,7 +190,11 @@ match_info_to_ip4_addr (GMatchInfo *matc
>      if (!bin || bin_len != 4)
>          goto done;
>  
> -    *out_addr = GUINT32_TO_BE (*((guint32 *) bin));
> +#if __BYTE_ORDER == __BIG_ENDIAN
> +    *out_addr = GUINT32_TO_LE (*((guint32 *) bin));
> +#else
> +    *out_addr = GUINT32_TO_BE (*((guint32 *) bin));   
> +#endif
>      success = TRUE;
>  
>  done:

I don't think there should be any need to check the __BYTE_ORDER explicitly. The original code assumed the hex str came with least significant byte first, so GUINT32_TO_BE () was trying  to convert from LE to BE, because inet address are given in BE.

The issue, I think, is that we're casting the output of mm_utils_hexstr2bin() directly to a host-endian uint32, which isn't explicitly LE.

Can you try this change instead in the BE systems?

    *out_addr = GUINT32_TO_BE (GUINT32_FROM_LE ((*((guint32 *) bin))));

Comment 6 Aleksander Morgado 2016-04-12 15:10:19 UTC

> 
> Can you try this change instead in the BE systems?
> 
>     *out_addr = GUINT32_TO_BE (GUINT32_FROM_LE ((*((guint32 *) bin))));

Or better, likely:

    *out_addr = GUINT32_SWAP_LE_BE ((*((guint32 *) bin)));

Comment 7 than 2016-04-13 09:18:03 UTC

(In reply to Aleksander Morgado from comment #6)
> > 
> > Can you try this change instead in the BE systems?
> > 
> >     *out_addr = GUINT32_TO_BE (GUINT32_FROM_LE ((*((guint32 *) bin))));
> 
> Or better, likely:
> 
>     *out_addr = GUINT32_SWAP_LE_BE ((*((guint32 *) bin)));

yes, you are right, the check is not needed here. GUINT32_SWAP_LE_BE works just fine.

diff -up ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c.than ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c
--- ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c.than  2016-04-12 08:15:09.866846700 -0400
+++ ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c       2016-04-13 05:16:08.341392975 -0400
@@ -190,7 +190,7 @@ match_info_to_ip4_addr (GMatchInfo *matc
     if (!bin || bin_len != 4)
         goto done;
 
-    *out_addr = GUINT32_TO_BE (*((guint32 *) bin));
+    *out_addr = GUINT32_SWAP_LE_BE (*((guint32 *) bin));
     success = TRUE;
 
 done:

Comment 8 Aleksander Morgado 2016-04-13 09:41:07 UTC

(In reply to than from comment #7)
> (In reply to Aleksander Morgado from comment #6)
> > > 
> > > Can you try this change instead in the BE systems?
> > > 
> > >     *out_addr = GUINT32_TO_BE (GUINT32_FROM_LE ((*((guint32 *) bin))));
> > 
> > Or better, likely:
> > 
> >     *out_addr = GUINT32_SWAP_LE_BE ((*((guint32 *) bin)));
> 
> yes, you are right, the check is not needed here. GUINT32_SWAP_LE_BE works
> just fine.
> 
> diff -up ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c.than
> ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c
> --- ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c.than 
> 2016-04-12 08:15:09.866846700 -0400
> +++ ModemManager-1.5.991/plugins/huawei/mm-modem-helpers-huawei.c      
> 2016-04-13 05:16:08.341392975 -0400
> @@ -190,7 +190,7 @@ match_info_to_ip4_addr (GMatchInfo *matc
>      if (!bin || bin_len != 4)
>          goto done;
>  
> -    *out_addr = GUINT32_TO_BE (*((guint32 *) bin));
> +    *out_addr = GUINT32_SWAP_LE_BE (*((guint32 *) bin));
>      success = TRUE;
>  
>  done:

Thanks, pushed to git master.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.