Re: [RFC] sg3_utils: sgp_dd: work around on pthread_cancel for android

Re: [RFC] sg3_utils: sgp_dd: work around on pthread_cancel for android

[Bean Huo (beanhuo)]

Hi, Doug and Bart
Thanks so much for your prompt response.
See below my action:

>On 2017-12-22 01:24 PM, Bart Van Assche wrote:
>> On Fri, 2017-12-22 at 17:00 +0000, Bean Huo (beanhuo) wrote:
>>>   case "${host}" in
>>> +        *-*-android*)
>>> +		AC_DEFINE_UNQUOTED(SG_ON_ANDROID, 1, [sg3_utils on
>android])
>>> +		AC_DEFINE_UNQUOTED(SG_LIB_LINUX, 1, [sg3_utils on linux])
>>> +                AC_SUBST([os_cflags], [''])
>>> +                AC_SUBST([os_libs], ['']) ;;
>>>           *-*-linux-gnu*)
>>>   		AC_DEFINE_UNQUOTED(SG_LIB_LINUX, 1, [sg3_utils on linux])
>>>                   AC_SUBST([os_cflags], ['']) @@ -79,6 +84,7 @@
>>> AM_CONDITIONAL(OS_OSF, [echo $host_os | grep '^osf' > /dev/null])
>>>   AM_CONDITIONAL(OS_SOLARIS, [echo $host_os | grep '^solaris' >
>/dev/null])
>>>   AM_CONDITIONAL(OS_WIN32_MINGW, [echo $host_os | grep '^mingw' >
>/dev/null])
>>>   AM_CONDITIONAL(OS_WIN32_CYGWIN, [echo $host_os | grep '^cygwin' >
>>> /dev/null])
>>> +AM_CONDITIONAL(OS_ANDROID, [echo $host_os | grep 'android' >
>>> +/dev/null])
>>
>> Hello Bean,
>>
>> Please consider to use AC_CHECK_FUNC([pthread_cancel]) or similar to
>> check the availability of pthread_cancel(). Explicit distro checks are
>> much harder to maintain than checks for individual functions.
>
>That macro doesn't work in Linux. It doesn't work in the sense that Linux
>(Ubuntu 17.10) does have pthread_cancel but needs -lpthread in the link. So
>AC_CHECK_FUNC([pthread_cancel]) does not place '#define
>HAVE_PTHREAD_CANCEL 1' in config.h .
>
Correct, pthread_cancel needs -lpthread in the link, AC_CHECK_FUNC cannot
Properly figure out that.

>This seems to work in Linux but may not work in Android:
>AC_SEARCH_LIBS([pthread_cancel], [pthread],
>[AC_DEFINE(HAVE_PTHREAD_CANCEL, 1, [Found pthread_cancel])], [])
>
>Bean, could you check?
>
Doug, I had tried that, it works, but this is for C codes macro definition in the config.h.
How can I figure out in the /src/Makefile.ac between Android OS and non-Android?
Can you give me some suggestions? Like this:
     if OS_ANDROID
     sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@
    else
    sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@ -lpthread
    endif
>Anyway, I like having an OS type for Android (which I have renamed from
>Bean's patch to SG_LIB_ANDROID to be consistent with my other OS naming).
>Overall the patch works in Linux.
>
Yes, we still need this OS type for Android, since there are several codes needed to be
Modified based on this OS type. For example, block device node folder in the Android
is /dev/block/sgx..., but for the other Linux based OS, it is /dev/sgx....

Have a great holiday!
Bean Huo

Re: [RFC] sg3_utils: sgp_dd: work around on pthread_cancel for android

[Bart Van Assche]

On Sat, 2017-12-23 at 10:19 +0000, Bean Huo (beanhuo) wrote:
> Doug wrote:
> > This seems to work in Linux but may not work in Android:
> > AC_SEARCH_LIBS([pthread_cancel], [pthread],
> > [AC_DEFINE(HAVE_PTHREAD_CANCEL, 1, [Found pthread_cancel])], [])
> > 
> > Bean, could you check?
> > 
> 
> Doug, I had tried that, it works, but this is for C codes macro definition in the config.h.
> How can I figure out in the /src/Makefile.ac between Android OS and non-Android?
> Can you give me some suggestions? Like this:
>      if OS_ANDROID
>      sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@
>     else
>     sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@ -lpthread
>     endif

How about leaving out the following:
- Use AC_SEARCH_LIBS([pthread_cancel], [pthread]) and AC_CHECK_FUNC([pthread_cancel]) in
  configure.ac. 
- Use @LIBS@ in Makefile.am.

From https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Libraries.html:
"If action-if-found is not specified, the default action prepends -llibrary to LIBS [...]".

Bart.

Re: [RFC] sg3_utils: sgp_dd: work around on pthread_cancel for android

[Bean Huo (beanhuo)]

Hi, Bart

>
>On Sat, 2017-12-23 at 10:19 +0000, Bean Huo (beanhuo) wrote:
>> Doug wrote:
>> > This seems to work in Linux but may not work in Android:
>> > AC_SEARCH_LIBS([pthread_cancel], [pthread],
>> > [AC_DEFINE(HAVE_PTHREAD_CANCEL, 1, [Found pthread_cancel])], [])
>> >
>> > Bean, could you check?
>> >
>>
>> Doug, I had tried that, it works, but this is for C codes macro definition in
>the config.h.
>> How can I figure out in the /src/Makefile.ac between Android OS and non-
>Android?
>> Can you give me some suggestions? Like this:
>>      if OS_ANDROID
>>      sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@
>>     else
>>     sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@ -lpthread
>>     endif
>
>How about leaving out the following:
>- Use AC_SEARCH_LIBS([pthread_cancel], [pthread]) and
>AC_CHECK_FUNC([pthread_cancel]) in
>  configure.ac.
>- Use @LIBS@ in Makefile.am.
>
>From https://www.gnu.org/software/autoconf/manual/autoconf-
>2.66/html_node/Libraries.html:
>"If action-if-found is not specified, the default action prepends -llibrary to LIBS
>[...]".
I checked these. My changes are below:

Configure.ac:
+# check for pthread_cancel
+AC_SEARCH_LIBS(pthread_cancel, pthread,
+     [AC_DEFINE(HAVE_PTHREAD_CANCEL, 1, [have pthread_cancel])], [], [])

Src/Makefile.am
-sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@ -lpthread
+sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@ @LIBS@

In the Src/sgp_dd.c, I can use macro definition " HAVE_PTHREAD_CANCEL" to differentiate
pthread_cancel supports or not.

It works. 
But I got one warning below while run autoreconf , do you know what is wrong with that?
configure.ac:15: warning: AC_PROG_LIBTOOL was called before AM_PROG_AR
configure.ac:15: warning: LT_INIT was called before AM_PROG_AR

thanks in advance!
Bean Huo

Re: [RFC] sg3_utils: sgp_dd: work around on pthread_cancel for android

[Bart Van Assche]

On 12/26/17 14:21, Bean Huo (beanhuo) wrote:
>My changes are below:
> 
> Configure.ac:
> +# check for pthread_cancel
> +AC_SEARCH_LIBS(pthread_cancel, pthread,
> +     [AC_DEFINE(HAVE_PTHREAD_CANCEL, 1, [have pthread_cancel])], [], [])
> 
> Src/Makefile.am
> -sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@ -lpthread
> +sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@ @LIBS@
> 
> In the Src/sgp_dd.c, I can use macro definition " HAVE_PTHREAD_CANCEL" to differentiate
> pthread_cancel supports or not.
> 
> It works.
> But I got one warning below while run autoreconf , do you know what is wrong with that?
> configure.ac:15: warning: AC_PROG_LIBTOOL was called before AM_PROG_AR
> configure.ac:15: warning: LT_INIT was called before AM_PROG_AR

Hello Bean,

Even if these warnings would get addressed, we still would need 
something like the following change for src/sgp_dd.c:

diff --git a/src/sgp_dd.c b/src/sgp_dd.c
--- a/src/sgp_dd.c
+++ b/src/sgp_dd.c
@@ -181,6 +181,7 @@ static int sg_finish_io(bool wr, Rq_elem * rep,

  static pthread_mutex_t strerr_mut = PTHREAD_MUTEX_INITIALIZER;

+static bool shutting_down = false;
  static bool do_sync = false;
  static bool do_time = false;
  static Rq_coll rcoll;
@@ -470,6 +471,8 @@ sig_listen_thread(void * v_clp)

      while (1) {
          sigwait(&signal_set, &sig_number);
+        if (shutting_down)
+                break;
          if (SIGINT == sig_number) {
              pr2serr(ME "interrupted by SIGINT\n");
              guarded_stop_both(clp);
@@ -1629,8 +1632,10 @@ main(int argc, char * argv[])
          }
      }

-    status = pthread_cancel(sig_listen_thread_id);
-    if (0 != status) err_exit(status, "pthread_cancel");
+    shutting_down = true;
+
+    status = pthread_kill(sig_listen_thread_id, SIGINT);
+    if (0 != status) err_exit(status, "pthread_kill");
      if (STDIN_FILENO != rcoll.infd)
          close(rcoll.infd);

My proposal is to leave out any ifdef HAVE_PTHREAD_CANCEL tests and to 
use pthread_kill() on all platforms. Since sgp_dd is the only tool that 
uses pthread_cancel() this approach will allow to leave out the 
pthread_cancel() configure test. I'm proposing this approach because it 
will make it easier to maintain and to test the sgp_dd tool.

Thanks,

Bart.

[RFC] sg3_utils: sgp_dd: work around on pthread_cancel for android

[Bean Huo (beanhuo)]

pthread_cancel is not supported by Android Bionic C library
pthread. Based on my searched information online, pthread_cancel
sometimes is likely to lead to memory leaks and dead locks.
If we use android NDK or standalone toolchain to cross complile
sg3_utils, below failure appeared.

    "....aarch64-linux-android/bin/ld: cannot find -lpthread
    collect2: error: ld returned 1 exit status
    Makefile:990: recipe for target 'sgp_dd' failed"

This patch is using signal to replace the cancel call when host_os is
android system.

Signed-off-by: beanhuo <beanhuo@micron.com>
---
 configure       | 30 ++++++++++++++++++++++++++++++
 configure.ac    |  6 ++++++
 src/Makefile.am |  4 ++++
 src/sgp_dd.c    | 17 +++++++++++++++++
 4 files changed, 57 insertions(+)

diff --git a/configure b/configure
index 9958be2..d9f23f2 100755
--- a/configure
+++ b/configure
@@ -635,6 +635,8 @@ ac_subst_vars='am__EXEEXT_FALSE
 am__EXEEXT_TRUE
 LTLIBOBJS
 LIBOBJS
+OS_ANDROID_FALSE
+OS_ANDROID_TRUE
 OS_WIN32_CYGWIN_FALSE
 OS_WIN32_CYGWIN_TRUE
 OS_WIN32_MINGW_FALSE
@@ -12292,6 +12294,21 @@ _ACEOF
 
 
 case "${host}" in
+        *-*-android*)
+
+cat >>confdefs.h <<_ACEOF
+#define SG_ON_ANDROID 1
+_ACEOF
+
+
+cat >>confdefs.h <<_ACEOF
+#define SG_LIB_LINUX 1
+_ACEOF
+
+                os_cflags=''
+
+                os_libs=''
+ ;;
         *-*-linux-gnu*)
 
 cat >>confdefs.h <<_ACEOF
@@ -12428,6 +12445,14 @@ else
   OS_WIN32_CYGWIN_FALSE=
 fi
 
+ if echo $host_os | grep 'android' > /dev/null; then
+  OS_ANDROID_TRUE=
+  OS_ANDROID_FALSE='#'
+else
+  OS_ANDROID_TRUE='#'
+  OS_ANDROID_FALSE=
+fi
+
 
 # Check whether --enable-linuxbsg was given.
 if test "${enable_linuxbsg+set}" = set; then :
@@ -12624,6 +12649,10 @@ if test -z "${OS_WIN32_CYGWIN_TRUE}" && test -z "${OS_WIN32_CYGWIN_FALSE}"; then
   as_fn_error $? "conditional \"OS_WIN32_CYGWIN\" was never defined.
 Usually this means the macro was only invoked conditionally." "$LINENO" 5
 fi
+if test -z "${OS_ANDROID_TRUE}" && test -z "${OS_ANDROID_FALSE}"; then
+  as_fn_error $? "conditional \"OS_ANDROID\" was never defined.
+Usually this means the macro was only invoked conditionally." "$LINENO" 5
+fi
 
 : "${CONFIG_STATUS=./config.status}"
 ac_write_fail=0
@@ -14211,6 +14240,7 @@ $as_echo X"$file" |
     cat <<_LT_EOF >> "$cfgfile"
 #! $SHELL
 # Generated automatically by $as_me ($PACKAGE) $VERSION
+# Libtool was configured on host `(hostname || uname -n) 2>/dev/null | sed 1q`:
 # NOTE: Changes made to this file will be lost: look at ltmain.sh.
 
 # Provide generalized library-building support services.
diff --git a/configure.ac b/configure.ac
index 6c35d1a..cef0686 100644
--- a/configure.ac
+++ b/configure.ac
@@ -37,6 +37,11 @@ AC_CANONICAL_HOST
 AC_DEFINE_UNQUOTED(SG_LIB_BUILD_HOST, "${host}", [sg3_utils Build Host])
 
 case "${host}" in
+        *-*-android*)
+		AC_DEFINE_UNQUOTED(SG_ON_ANDROID, 1, [sg3_utils on android])
+		AC_DEFINE_UNQUOTED(SG_LIB_LINUX, 1, [sg3_utils on linux])
+                AC_SUBST([os_cflags], [''])
+                AC_SUBST([os_libs], ['']) ;;
         *-*-linux-gnu*)
 		AC_DEFINE_UNQUOTED(SG_LIB_LINUX, 1, [sg3_utils on linux])
                 AC_SUBST([os_cflags], [''])
@@ -79,6 +84,7 @@ AM_CONDITIONAL(OS_OSF, [echo $host_os | grep '^osf' > /dev/null])
 AM_CONDITIONAL(OS_SOLARIS, [echo $host_os | grep '^solaris' > /dev/null])
 AM_CONDITIONAL(OS_WIN32_MINGW, [echo $host_os | grep '^mingw' > /dev/null])
 AM_CONDITIONAL(OS_WIN32_CYGWIN, [echo $host_os | grep '^cygwin' > /dev/null])
+AM_CONDITIONAL(OS_ANDROID, [echo $host_os | grep 'android' > /dev/null])
 
 AC_ARG_ENABLE([linuxbsg],
   AC_HELP_STRING([--disable-linuxbsg], [ignore linux bsg (sgv4) if present]),
diff --git a/src/Makefile.am b/src/Makefile.am
index 1012a78..daada79 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -86,7 +86,11 @@ sg_modes_LDADD = ../lib/libsgutils2.la @os_libs@
 
 sg_opcodes_LDADD = ../lib/libsgutils2.la @os_libs@
 
+if OS_ANDROID
+sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@
+else
 sgp_dd_LDADD = ../lib/libsgutils2.la @os_libs@ -lpthread
+endif
 
 sg_persist_LDADD = ../lib/libsgutils2.la @os_libs@
 
diff --git a/src/sgp_dd.c b/src/sgp_dd.c
index e154555..d59167c 100644
--- a/src/sgp_dd.c
+++ b/src/sgp_dd.c
@@ -1121,6 +1121,11 @@ process_flags(const char * arg, struct flags_t * fp)
     return 0;
 }
 
+void
+thread_exit_handler(int sig)
+{
+    pthread_exit(0);
+}
 
 #define STR_SZ 1024
 #define INOUTF_SZ 512
@@ -1147,6 +1152,14 @@ main(int argc, char * argv[])
     int in_sect_sz, out_sect_sz, status, n, flags;
     void * vp;
     char ebuff[EBUFF_SZ];
+#if SG_ON_ANDROID
+    struct sigaction actions;
+    memset(&actions, 0, sizeof(actions));
+    sigemptyset(&actions.sa_mask);
+    actions.sa_flags = 0;
+    actions.sa_handler = thread_exit_handler;
+    sigaction(SIGUSR1,&actions,NULL);
+#endif
 
     memset(&rcoll, 0, sizeof(Rq_coll));
     rcoll.bpt = DEF_BLOCKS_PER_TRANSFER;
@@ -1629,7 +1642,11 @@ main(int argc, char * argv[])
         }
     }
 
+#if SG_ON_ANDROID
+    status = pthread_kill(sig_listen_thread_id, SIGUSR1);
+#else
     status = pthread_cancel(sig_listen_thread_id);
+#endif
     if (0 != status) err_exit(status, "pthread_cancel");
     if (STDIN_FILENO != rcoll.infd)
         close(rcoll.infd);
-- 
2.7.4

Re: [RFC] sg3_utils: sgp_dd: work around on pthread_cancel for android

[Bart Van Assche]

On Fri, 2017-12-22 at 17:00 +0000, Bean Huo (beanhuo) wrote:
>  case "${host}" in
> +        *-*-android*)
> +		AC_DEFINE_UNQUOTED(SG_ON_ANDROID, 1, [sg3_utils on android])
> +		AC_DEFINE_UNQUOTED(SG_LIB_LINUX, 1, [sg3_utils on linux])
> +                AC_SUBST([os_cflags], [''])
> +                AC_SUBST([os_libs], ['']) ;;
>          *-*-linux-gnu*)
>  		AC_DEFINE_UNQUOTED(SG_LIB_LINUX, 1, [sg3_utils on linux])
>                  AC_SUBST([os_cflags], [''])
> @@ -79,6 +84,7 @@ AM_CONDITIONAL(OS_OSF, [echo $host_os | grep '^osf' > /dev/null])
>  AM_CONDITIONAL(OS_SOLARIS, [echo $host_os | grep '^solaris' > /dev/null])
>  AM_CONDITIONAL(OS_WIN32_MINGW, [echo $host_os | grep '^mingw' > /dev/null])
>  AM_CONDITIONAL(OS_WIN32_CYGWIN, [echo $host_os | grep '^cygwin' > /dev/null])
> +AM_CONDITIONAL(OS_ANDROID, [echo $host_os | grep 'android' > /dev/null])

Hello Bean,

Please consider to use AC_CHECK_FUNC([pthread_cancel]) or similar to check the
availability of pthread_cancel(). Explicit distro checks are much harder to maintain
than checks for individual functions.

Thanks,

Bart.

Re: [RFC] sg3_utils: sgp_dd: work around on pthread_cancel for android

[Douglas Gilbert]

On 2017-12-22 01:24 PM, Bart Van Assche wrote:
> On Fri, 2017-12-22 at 17:00 +0000, Bean Huo (beanhuo) wrote:
>>   case "${host}" in
>> +        *-*-android*)
>> +		AC_DEFINE_UNQUOTED(SG_ON_ANDROID, 1, [sg3_utils on android])
>> +		AC_DEFINE_UNQUOTED(SG_LIB_LINUX, 1, [sg3_utils on linux])
>> +                AC_SUBST([os_cflags], [''])
>> +                AC_SUBST([os_libs], ['']) ;;
>>           *-*-linux-gnu*)
>>   		AC_DEFINE_UNQUOTED(SG_LIB_LINUX, 1, [sg3_utils on linux])
>>                   AC_SUBST([os_cflags], [''])
>> @@ -79,6 +84,7 @@ AM_CONDITIONAL(OS_OSF, [echo $host_os | grep '^osf' > /dev/null])
>>   AM_CONDITIONAL(OS_SOLARIS, [echo $host_os | grep '^solaris' > /dev/null])
>>   AM_CONDITIONAL(OS_WIN32_MINGW, [echo $host_os | grep '^mingw' > /dev/null])
>>   AM_CONDITIONAL(OS_WIN32_CYGWIN, [echo $host_os | grep '^cygwin' > /dev/null])
>> +AM_CONDITIONAL(OS_ANDROID, [echo $host_os | grep 'android' > /dev/null])
> 
> Hello Bean,
> 
> Please consider to use AC_CHECK_FUNC([pthread_cancel]) or similar to check the
> availability of pthread_cancel(). Explicit distro checks are much harder to maintain
> than checks for individual functions.

That macro doesn't work in Linux. It doesn't work in the sense that
Linux (Ubuntu 17.10) does have pthread_cancel but needs -lpthread
in the link. So AC_CHECK_FUNC([pthread_cancel]) does not place
'#define HAVE_PTHREAD_CANCEL 1' in config.h .

This seems to work in Linux but may not work in Android:
AC_SEARCH_LIBS([pthread_cancel], [pthread], [AC_DEFINE(HAVE_PTHREAD_CANCEL, 1, 
[Found pthread_cancel])], [])

Bean, could you check?

Anyway, I like having an OS type for Android (which I have renamed from
Bean's patch to SG_LIB_ANDROID to be consistent with my other OS
naming). Overall the patch works in Linux.

Doug Gilbert