EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

[Ken Brown]

The macro EMACS_GET_TTY_PGRP defined in src/systty.h uses either 
tcgetpgrp or TIOCGPGRP to get the PGID of a terminal's foreground 
process group.  But src/process.c defines and uses its own 
emacs_get_tty_pgrp that only works on systems that have TIOCGPGRP.  Is 
there a good reason for this?  If not, I would like to try to prepare a 
patch to change process.c to use the macro instead.  This would simplify 
the code and would also extend some of the functionality in process.c to 
systems that have tcgetpgrp but not TIOCGPGRP.

Ken

Re: EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

[Andreas Schwab]

Ken Brown <kbrown@cornell.edu> writes:

> The macro EMACS_GET_TTY_PGRP defined in src/systty.h uses either
> tcgetpgrp or TIOCGPGRP to get the PGID of a terminal's foreground
> process group.  But src/process.c defines and uses its own
> emacs_get_tty_pgrp that only works on systems that have TIOCGPGRP.  Is
> there a good reason for this?

Is "historical accident" a good reason? :-)

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

[Ken Brown]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

On 10/20/2010 11:14 AM, Andreas Schwab wrote:
> Ken Brown<kbrown@cornell.edu>  writes:
>
>> The macro EMACS_GET_TTY_PGRP defined in src/systty.h uses either
>> tcgetpgrp or TIOCGPGRP to get the PGID of a terminal's foreground
>> process group.  But src/process.c defines and uses its own
>> emacs_get_tty_pgrp that only works on systems that have TIOCGPGRP.  Is
>> there a good reason for this?
>
> Is "historical accident" a good reason? :-)

Sure.

It turned out that emacs_get_tty_pgrp has different arguments than its 
uppercase cousin, so I didn't get rid of it.  But I used the macro 
inside the function instead and got rid of all ifdefs involving 
TIOCGPGRP.  Does the attached patch look OK?

Ken

[-- Attachment #2: tty.patch --]
[-- Type: text/plain, Size: 1782 bytes --]

=== modified file 'src/process.c'
--- src/process.c	2010-09-30 04:28:34 +0000
+++ src/process.c	2010-10-20 17:32:57 +0000
@@ -6009,23 +6009,21 @@
 {
   int gid = -1;
 
-#ifdef TIOCGPGRP
-  if (ioctl (p->infd, TIOCGPGRP, &gid) == -1 && ! NILP (p->tty_name))
+  if (EMACS_GET_TTY_PGRP (p->infd, &gid) <= 0 && ! NILP (p->tty_name))
     {
       int fd;
-      /* Some OS:es (Solaris 8/9) does not allow TIOCGPGRP from the
-	 master side.  Try the slave side.  */
+      /* Some OS:es (Solaris 8/9) do not allow TIOCGPGRP/tcgetpgrp
+	 from the master side.  Try the slave side.  */
       fd = emacs_open (SDATA (p->tty_name), O_RDONLY, 0);
 
       if (fd != -1)
 	{
-	  ioctl (fd, TIOCGPGRP, &gid);
+	  EMACS_GET_TTY_PGRP (fd, &gid);
 	  emacs_close (fd);
 	}
     }
-#endif /* defined (TIOCGPGRP ) */
-
-  return gid;
+  /* On some systems EMACS_GET_TTY_PGRP can return 0 if it fails. */
+  return (gid > 0 ? gid : -1);
 }
 
 DEFUN ("process-running-child-p", Fprocess_running_child_p,
@@ -6206,7 +6204,7 @@
 	 handle the signal.  */
 #endif /* defined (SIGNALS_VIA_CHARACTERS) */
 
-#ifdef TIOCGPGRP
+#ifdef EMACS_HAVE_TTY_PGRP
       /* Get the current pgrp using the tty itself, if we have that.
 	 Otherwise, use the pty to get the pgrp.
 	 On pfa systems, saka@pfu.fujitsu.co.JP writes:
@@ -6227,11 +6225,11 @@
 	 Or perhaps this is vestigial.  */
       if (gid == -1)
 	no_pgrp = 1;
-#else  /* ! defined (TIOCGPGRP ) */
+#else  /* ! defined (EMACS_HAVE_TTY_PGRP) */
       /* Can't select pgrps on this system, so we know that
 	 the child itself heads the pgrp.  */
       gid = p->pid;
-#endif /* ! defined (TIOCGPGRP ) */
+#endif /* ! defined (EMACS_HAVE_TTY_PGRP) */
 
       /* If current_group is lambda, and the shell owns the terminal,
 	 don't send any signal.  */

Re: EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

[Dan Nicolaescu]

Ken Brown <kbrown@cornell.edu> writes:

> The macro EMACS_GET_TTY_PGRP defined in src/systty.h uses either
> tcgetpgrp or TIOCGPGRP to get the PGID of a terminal's foreground

It does not:

#ifndef DOS_NT
#define EMACS_GET_TTY_PGRP(fd, pgid) (*(pgid) = tcgetpgrp ((fd)))
#define EMACS_SET_TTY_PGRP(fd, pgid) (tcsetpgrp ((fd), *(pgid)))
#endif /* not DOS_NT */

also, I have a patch to remove this, it's only used in two places, so
it's not very useful as an abstraction.


> process group.  But src/process.c defines and uses its own
> emacs_get_tty_pgrp that only works on systems that have TIOCGPGRP.  Is
> there a good reason for this?  If not, I would like to try to prepare
> a patch to change process.c to use the macro instead.  This would
> simplify the code and would also extend some of the functionality in
> process.c to systems that have tcgetpgrp but not TIOCGPGRP.
>
> Ken

Re: EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

[Ken Brown]

On 10/20/2010 2:59 PM, Dan Nicolaescu wrote:
> Ken Brown<kbrown@cornell.edu>  writes:
>
>> The macro EMACS_GET_TTY_PGRP defined in src/systty.h uses either
>> tcgetpgrp or TIOCGPGRP to get the PGID of a terminal's foreground
>
> It does not:
>
> #ifndef DOS_NT
> #define EMACS_GET_TTY_PGRP(fd, pgid) (*(pgid) = tcgetpgrp ((fd)))
> #define EMACS_SET_TTY_PGRP(fd, pgid) (tcsetpgrp ((fd), *(pgid)))
> #endif /* not DOS_NT */
>
> also, I have a patch to remove this, it's only used in two places, so
> it's not very useful as an abstraction.

Oops.  I looked at the emacs-23 branch but not the trunk.  Sorry for the 
confusion.

But then I still think emacs_get_tty_pgrp in process.c should be changed 
so that it works on systems that have tcgetpgrp but not TIOCGPGRP.  How 
would you recommend handling this?  Is it necessary to use conditional 
code as in the emacs-23 definition of EMACS_GET_TTY_PGRP, or is it safe 
to just always use tcgetpgrp as in the trunk's version of 
EMACS_GET_TTY_PGRP?

Ken

Re: EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

[Eli Zaretskii]

> Date: Wed, 20 Oct 2010 16:01:42 -0400
> From: Ken Brown <kbrown@cornell.edu>
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> But then I still think emacs_get_tty_pgrp in process.c should be changed 
> so that it works on systems that have tcgetpgrp but not TIOCGPGRP.  How 
> would you recommend handling this?  Is it necessary to use conditional 
> code as in the emacs-23 definition of EMACS_GET_TTY_PGRP, or is it safe 
> to just always use tcgetpgrp as in the trunk's version of 
> EMACS_GET_TTY_PGRP?

I would suggest to use TIOCGPGRP (inside emacs_get_tty_pgrp) if it's
available, because at least on some systems it allows to access file
descriptors of processes other than the current one, while tcgetpgrp
does not.

Please note that emacs_get_tty_pgrp is used elsewhere in process.c,
and also make sure that it does what it does today on systems (w32)
which have neither TIOCGPGRP nor tcgetpgrp.

Re: EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

[Dan Nicolaescu]

Ken Brown <kbrown@cornell.edu> writes:

> On 10/20/2010 2:59 PM, Dan Nicolaescu wrote:
>> Ken Brown<kbrown@cornell.edu>  writes:
>>
>>> The macro EMACS_GET_TTY_PGRP defined in src/systty.h uses either
>>> tcgetpgrp or TIOCGPGRP to get the PGID of a terminal's foreground
>>
>> It does not:
>>
>> #ifndef DOS_NT
>> #define EMACS_GET_TTY_PGRP(fd, pgid) (*(pgid) = tcgetpgrp ((fd)))
>> #define EMACS_SET_TTY_PGRP(fd, pgid) (tcsetpgrp ((fd), *(pgid)))
>> #endif /* not DOS_NT */
>>
>> also, I have a patch to remove this, it's only used in two places, so
>> it's not very useful as an abstraction.
>
> Oops.  I looked at the emacs-23 branch but not the trunk.  Sorry for
> the confusion.
>
> But then I still think emacs_get_tty_pgrp in process.c should be
> changed so that it works on systems that have tcgetpgrp but not
> TIOCGPGRP.  How would you recommend handling this?  Is it necessary to
> use conditional code as in the emacs-23 definition of
> EMACS_GET_TTY_PGRP, or is it safe to just always use tcgetpgrp as in
> the trunk's version of EMACS_GET_TTY_PGRP?

It think your question compounds two issues.

EMACS_GET/SET_TTY_PGRP should die, they are not useful as abstractions anymore.

The comment in emacs_get_tty_pgrp looks scary, it does not look
obvious that the code in the function can be replaced with tcgetpgrp.
Maybe it can, but, IMHO, it needs testing on a few platforms.

Re: EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

[Ken Brown]

On 10/20/2010 4:28 PM, Dan Nicolaescu wrote:
> Ken Brown<kbrown@cornell.edu>  writes:
>
>> On 10/20/2010 2:59 PM, Dan Nicolaescu wrote:
>>> Ken Brown<kbrown@cornell.edu>   writes:
>>>
>>>> The macro EMACS_GET_TTY_PGRP defined in src/systty.h uses either
>>>> tcgetpgrp or TIOCGPGRP to get the PGID of a terminal's foreground
>>>
>>> It does not:
>>>
>>> #ifndef DOS_NT
>>> #define EMACS_GET_TTY_PGRP(fd, pgid) (*(pgid) = tcgetpgrp ((fd)))
>>> #define EMACS_SET_TTY_PGRP(fd, pgid) (tcsetpgrp ((fd), *(pgid)))
>>> #endif /* not DOS_NT */
>>>
>>> also, I have a patch to remove this, it's only used in two places, so
>>> it's not very useful as an abstraction.
>>
>> Oops.  I looked at the emacs-23 branch but not the trunk.  Sorry for
>> the confusion.
>>
>> But then I still think emacs_get_tty_pgrp in process.c should be
>> changed so that it works on systems that have tcgetpgrp but not
>> TIOCGPGRP.  How would you recommend handling this?  Is it necessary to
>> use conditional code as in the emacs-23 definition of
>> EMACS_GET_TTY_PGRP, or is it safe to just always use tcgetpgrp as in
>> the trunk's version of EMACS_GET_TTY_PGRP?
>
> It think your question compounds two issues.
>
> EMACS_GET/SET_TTY_PGRP should die, they are not useful as abstractions anymore.
>
> The comment in emacs_get_tty_pgrp looks scary, it does not look
> obvious that the code in the function can be replaced with tcgetpgrp.
> Maybe it can, but, IMHO, it needs testing on a few platforms.

I think I'm just going to let this drop.  I had thought I could simplify 
the code, but it looks like it would just get more complicated, and it's 
not worth the trouble.

Re: EMACS_GET_TTY_PGRP vs. emacs_get_tty_pgrp

[Andreas Schwab]

Dan Nicolaescu <dann@gnu.org> writes:

> The comment in emacs_get_tty_pgrp looks scary, it does not look
> obvious that the code in the function can be replaced with tcgetpgrp.
> Maybe it can, but, IMHO, it needs testing on a few platforms.

In all Unix systems that define both they should be identical
(ie. tcgetpgrp just calls the TIOCGPGRP ioctl).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."