Re: [PATCH 4/4] m4_esyscmd: fdopen() with a text mode explicitly on OS/2

Eric Blake <[email protected]> Wed, 19 Nov 2014 08:33:18 -0700

Newsgroups	gmane.comp.gnu.m4.patches
Organization	Red Hat, Inc.
Message-ID	<[email protected]>

On 11/18/2014 08:54 PM, KO Myung-Hun wrote:
> On OS/2 kLIBC, fdopen() creates a stream in a mode of a file
> descriptor. So specify "t" to open a stream in a text mode explicitly
> on OS/2.
> 
> * src/builtin.c (m4_esyscmd): fdopen() in a text mode on OS/2.
> ---
>  src/builtin.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/builtin.c b/src/builtin.c
> index e101838..7a73b36 100644
> --- a/src/builtin.c
> +++ b/src/builtin.c
> @@ -1019,7 +1019,12 @@ m4_esyscmd (struct obstack *obs, int argc, token_data **argv)
>        sysval = 127;
>        return;
>      }
> -  pin = fdopen (fd, "r");
> +#if OS2
> +# define MODE_TEXT "t"
> +#else
> +# define MODE_TEXT ""
> +#endif

Eww.  Mid-function #ifdefs are evil.  I strongly prefer that we hoist it
out of the function.  Also, fdopen("rt") looks awkward, since 't' mode
is non-standard.  Why do you need text mode?  What is the default if you
omit 't', and why is binary mode not good enough?  I'm very reluctant to
see this patch applied as-is without more reasoning and/or more
refactoring for easier maintenance.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

_______________________________________________
M4-patches mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/m4-patches

signature.asc (application/pgp-signature, 539 B)