Re: [Qemu-devel] [RFC PATCH v4] os-android: Add support to android platform

[Stefan Hajnoczi <stefanha@gmail.com>]

On Sat, Oct 03, 2015 at 12:44:14PM +0800, Houcheng Lin wrote:
> diff --git a/configure b/configure
> index d7c24cd..cda88c1 100755
> --- a/configure
> +++ b/configure
> @@ -567,7 +567,6 @@ fi
>  
>  # host *BSD for user mode
>  HOST_VARIANT_DIR=""
> -
>  case $targetos in
>  CYGWIN*)
>    mingw32="yes"

Spurious whitespace change

> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index b1beaa6..44beee3 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -22,7 +22,6 @@
>   */
>  #include <stdio.h>
>  #include <unistd.h>
> -#include <sys/io.h>
>  #include <sys/mman.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>

What is the justification for this?  Do you know why io.h was included
before?

> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index ab3c876..9e26d10 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -74,6 +74,14 @@ typedef unsigned int            uint_fast16_t;
>  typedef signed int              int_fast16_t;
>  #endif
>  
> +#ifdef CONFIG_ANDROID
> +/*
> + * For include the basename prototyping in android.
> + */
> +#include <libgen.h>

Files that use basename(3) should include libgen.h.  Why include it
here?

> +#define IOV_MAX 1024

Are you sure that Android NDK headers do not contain this constant?

> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 3ae4987..4ae746b 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -62,6 +62,8 @@ extern int daemon(int, int);
>  #include <libgen.h>
>  #include <setjmp.h>
>  #include <sys/signal.h>
> +#include <sys/time.h>

Why did you include time.h?

> +#include <sys/resource.h>
>  
>  #ifdef CONFIG_LINUX
>  #include <sys/syscall.h>
> @@ -482,3 +484,17 @@ int qemu_read_password(char *buf, int buf_size)
>      printf("\n");
>      return ret;
>  }
> +
> +int qemu_getdtablesize(void)
> +{
> +#ifdef CONFIG_ANDROID
> +    struct rlimit r;
> +
> +    if (getrlimit(RLIMIT_NOFILE, &r) < 0) {
> +        return sysconf(_SC_OPEN_MAX);
> +    }
> +    return r.rlim_cur;
> +#else
> +    return getdtablesize();
> +#endif
> +}

We can probably drop the getdtablesize() call completely and use the
CONFIG_ANDROID code on all platforms.  I suggest splitting this out into
a separate patch that introduces qemu_getdtablesize() and converts all
callers.

> diff --git a/util/qemu-openpty.c b/util/qemu-openpty.c
> index 4c53211..b305886 100644
> --- a/util/qemu-openpty.c
> +++ b/util/qemu-openpty.c
> @@ -51,12 +51,17 @@
>  # include <termios.h>
>  #endif
>  
> -#ifdef __sun__
> +#if defined(__sun__) || defined(CONFIG_ANDROID)
> +
>  /* Once Solaris has openpty(), this is going to be removed. */
>  static int openpty(int *amaster, int *aslave, char *name,
>                     struct termios *termp, struct winsize *winp)
>  {
> +#if defined(CONFIG_ANDROID)
> +        char slave[PATH_MAX];
> +#else
>          const char *slave;
> +#endif
>          int mfd = -1, sfd = -1;
>  
>          *amaster = *aslave = -1;
> @@ -67,17 +72,22 @@ static int openpty(int *amaster, int *aslave, char *name,
>  
>          if (grantpt(mfd) == -1 || unlockpt(mfd) == -1)
>                  goto err;
> -
> +#if defined(CONFIG_ANDROID)
> +        if (ptsname_r(mfd, slave, PATH_MAX) < 0)
> +                goto err;
> +#else
>          if ((slave = ptsname(mfd)) == NULL)
>                  goto err;
> +#endif

ptsname_r(3) should be used on all Linux hosts because it is reentrant.
This improvement isn't Android-specific, please split it into a separate
patch.