Date: Fri, 1 May 2015 16:20:40 +1000
From: NeilBrown <neilb@suse.de>
To: Peter Hurley <peter@hurleysoftware.com>
Cc: Michael Matz <matz@suse.de>, Nic Percival <Nic.Percival@microfocus.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.cz>, linux-kernel@vger.kernel.org
Subject: [PATCH bisected regression] input_available_p() sometimes says 'no'
 when it should say 'yes'
Message-ID: <20150501162040.05c0cb42@notabene.brown>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
 boundary="Sig_/YsmFTzy/q09G/2kW.vZ1AHi"; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 9726
Lines: 402

--Sig_/YsmFTzy/q09G/2kW.vZ1AHi
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable


Hi Peter,
 I recently had a report of a regression in 3.12.  I bisected it down to yo=
ur
 patch
  f95499c3030f ("n_tty: Don't wait for buffer work in read() loop")

 Sometimes a poll on a master-pty will report there is nothing to read after
 the slave has written something.
 As test program is below.
 On a kernel prior to your commit, this program never reports

Total bytes read is 0. PollRC=3D0

 On a kernel subsequent to your commit, that message is produced quite ofte=
n.

 This was found while working on a debugger.

 Following the test program is my proposed patch which allows the program to
 run as it used to.  It re-introduces the call to tty_flush_to_ldisc(), but
 only if it appears that there is nothing to read.

 Do you think this is a suitable fix?  Do you even agree that it is a real
 bug?

Thanks,
NeilBrown


------------------------------------------------------
#define _XOPEN_SOURCE
#include<unistd.h>
#include<stdlib.h>
#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<errno.h>
#include<sys/wait.h>
#include<sys/types.h>
#include<sys/ptrace.h>
#include<asm/ptrace.h>
#include<fcntl.h>
#include<sys/poll.h>


#define USEPTY
#define COUNT_MAX	(5000000)
#define MY_BREAKPOINT	{ asm("int $3"); }
#define PTRACE_IGNORED	(void *)0

/*
** Open s pseudo-tty pair.
**
** Return the master fd, set spty to the slave fd.
*/
int
my_openpt(int *spty)
{
    int mfd =3D posix_openpt(O_RDWR | O_NOCTTY);
    char *slavedev;
    int sfd=3D-1;
    if(mfd =3D=3D -1) return -1;
    if(grantpt(mfd) =3D=3D -1) return -1;
    if(unlockpt(mfd) =3D=3D -1) return -1;

    slavedev =3D (char *)ptsname(mfd);

    if((sfd =3D open(slavedev, O_RDWR | O_NOCTTY)) =3D=3D -1)
    {
	close(mfd);
	return -1;
    }
    if(spty !=3D NULL)
    {
	*spty =3D sfd;
    }
    return mfd;
}


/*
** Read from the provided file descriptor if poll says there's
** anything there..
*/
int
DoPollRead(int mpty)
{
    struct pollfd fds;
    int pollrc;
    ssize_t bread=3D0, totread=3D0;
    char readbuf[101];

    /*
    ** Set up the poll.
    */
    fds.fd =3D mpty;
    fds.events =3D POLLIN | POLLRDNORM | POLLRDBAND | POLLPRI;

    /*
    ** poll for any output.
    */
   =20
    while((pollrc =3D poll(&fds, 1, 0)) =3D=3D 1)
    {
	if(fds.revents & POLLIN)
	{
	    bread =3D read( mpty, readbuf, 100 );
	    totread +=3D bread;
	    if(bread > 0)
	    {
		//printf("Read %d bytes.\n", (int)bread);
		readbuf[bread] =3D '\0';
		//printf("\t%s", readbuf);
	    } else
	    {
		//puts("Nothing read.\n");
	    }
	} else if (fds.revents & (POLLHUP | POLLERR | POLLNVAL)) {
	    printf ("hangup/error/invalid on poll\n");
	    return totread;
	} else { printf("No POLLIN, revents=3D%d\n", fds.revents); };
    }

    /*
    ** This sometimes happens - we're expecting input on the pty,=20
    ** but nothing is there.
    */
    if(totread =3D=3D 0)
	printf("Total bytes read is 0. PollRC=3D%d\n", pollrc);

    return totread;
}

static
void writeall (int fd, const char *buf, size_t count)
{
  while (count)
    {
      ssize_t r =3D write (fd, buf, count);
      if (r =3D=3D 0)
	break;
      if (r < 0 && errno =3D=3D EINTR)
	continue;
      if (r < 0)
	exit (2);
      count -=3D r;
      buf +=3D r;
    }
}

int
thechild(void)
{
    unsigned int i;

    writeall (1, "debuggee starts\n", strlen ("debuggee starts\n"));

    for(i=3D0 ; i<COUNT_MAX ; i++)
    {
        char buf[100];
	sprintf(buf, "This is the debuggee. Count is %d\n", i);
	writeall (1, buf, strlen (buf));

	MY_BREAKPOINT
    }

    writeall (1, "debuggee finishing now.\n", strlen ("debuggee finishing n=
ow.\n"));
    exit (0);
}

int
main()
{
    int rv, status, i=3D0;
    pid_t pid;
    int sfd =3D -1;
    int mfd;
#ifdef USEPTY
    mfd =3D my_openpt(&sfd);	/* Get a pseudo-tty pair. */
    if(mfd < 0)
    {
	fprintf(stderr, "Failed to create pty\n");
	return(1);
    }
#else
    int pipefd[2];
    if (pipe (pipefd) < 0)
      {
	perror ("pipe");
	return 1;
      }
    mfd =3D pipefd[0];
    sfd =3D pipefd[1];
#endif

    /*
    ** Create a child process.
    */
    pid =3D fork();
    switch(pid)
    {
	case -1:	/* failed fork		*/
	    return -1;
	case 0:		/* child process	*/

	    close (mfd);
	    /*
	    ** Close stdout, use the slave pty for output.
	    */
	    dup2(sfd, STDOUT_FILENO);


	    /*
	    ** Set 'TRACEME' so this child process can be traced by the
	    ** parent process.=20
	    */
	    ptrace(PTRACE_TRACEME,
			PTRACE_IGNORED, PTRACE_IGNORED, PTRACE_IGNORED);
	    thechild ();
	    break;

	default:	/* parent process drops out of switch	*/
	    close (sfd);
	    break;
    }

    /*
    ** Wait for the debuggee to hit the traceme.
    ** When we see this, immediately send a PTRACE_CONT to kick off
    ** the debuggee..
    */
    rv =3D waitpid(pid, &status, 0);
    if(WIFSTOPPED(status))
    {
	printf("Debuggee initial stop. Sending PTRACE_CONT..\n");
	ptrace(PTRACE_CONT, pid, PTRACE_IGNORED,PTRACE_IGNORED);
    }
    else
    {
	printf("Failure of debuggee to hit initial 'traceme'\n");
	return 99;
    }

    /*
    ** Loop around, waiting for the debuggee to hit its own breakpoint.
    ** Look for output on the pty. Each time around we should see a line
    ** from the debuggee.
    **/
    while(1)
    {
	int pollrc, stopped, exited;

	//printf("Debugger doing a waitpid on debuggee..(%d)\n", i);
	i++;

	rv =3D waitpid(pid, &status, 0);
	stopped=3D0;
	exited=3D0;

	/*
	** Ok, the waitpid has returned. What's happened to the debuggee?
	** Note we do recheck rv and drop out later if it's -1.
	*/
	if(rv =3D=3D -1)
	{
	    printf("Debuggee process has died?. Checking for output.\n");
	}
	else if(WIFSTOPPED(status))
	{
	    //printf("Debuggee process has stopped. Checking for output.\n");
	    stopped=3D1;
	}
	else if(!WIFEXITED(status))
	{
	    printf("Debuggee has exited. Checking for output.\n");
	    exited=3D1;
	}
	else
	{
	    printf("WEXITSTATUS is %d\n", WEXITSTATUS(status));
	    exited=3D1;
	}

	/*
	** See if there's anything to read. If so display it.
	** There always should be, the debuggee writes output on each
	** iteration and fflush's it.
	*/
	(void) DoPollRead(mfd);

	/*
	** If the debuggee has stopped tell it to continue. If it's
	** exited detach from it.
	*/
	if(stopped)
	{
	    //puts("Sending PTRACE_CONT");
	    ptrace(PTRACE_CONT, pid, PTRACE_IGNORED,PTRACE_IGNORED);
	} else if(exited)
	{
	    printf("Debuggee exited, detaching\n");
	    ptrace(PTRACE_DETACH, pid, PTRACE_IGNORED,PTRACE_IGNORED);
	    return 0;
	} else if(rv =3D=3D -1)
	{
	    printf("Debuggee died? Leaving.\n");
	    return 98;
	}
    }
}
------------------------------------------------------


From: NeilBrown <neilb@suse.de>
Subject: [PATCH] n_tty: Sometimes wait for buffer work in read() loop

Since commit
  f95499c3030f ("n_tty: Don't wait for buffer work in read() loop")

it as been possible for poll to report that there is no data to read
on a master-pty even if a write to the slave has actually completed.

That patch removes a 'wait' when the wait isn't really necessary.
Unfortunately it also removed it in the case when it *is* necessary.
If the simple tests show that there is nothing to read, we really need
to flush the work queue in case there is something ready but which
hasn't arrived yet.

This patch restores the wait, but only if simple tests suggest there
is nothing ready.

Reported-by: Nic Percival <Nic.Percival@microfocus.com>
Reported-by: Michael Matz <matz@suse.de>
Fixes: f95499c3030f ("n_tty: Don't wait for buffer work in read() loop")
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index cf6e0f2e1331..9884091819b6 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1942,11 +1942,18 @@ static inline int input_available_p(struct tty_stru=
ct *tty, int poll)
 {
 	struct n_tty_data *ldata =3D tty->disc_data;
 	int amt =3D poll && !TIME_CHAR(tty) && MIN_CHAR(tty) ? MIN_CHAR(tty) : 1;
-
-	if (ldata->icanon && !L_EXTPROC(tty))
-		return ldata->canon_head !=3D ldata->read_tail;
-	else
-		return ldata->commit_head - ldata->read_tail >=3D amt;
+	int i;
+	int ret =3D 0;
+
+	for (i =3D 0; !ret && i < 2; i++) {
+		if (i)
+			tty_flush_to_ldisc(tty);
+		if (ldata->icanon && !L_EXTPROC(tty))
+			ret =3D (ldata->canon_head !=3D ldata->read_tail);
+		else
+			ret =3D (ldata->commit_head - ldata->read_tail >=3D amt);
+	}
+	return ret;
 }
=20
 /**

--Sig_/YsmFTzy/q09G/2kW.vZ1AHi
Content-Type: application/pgp-signature
Content-Description: OpenPGP digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIVAwUBVUMbODnsnt1WYoG5AQIo4Q/6A93D9Mh1Btt6PY2S7CqQwlEkC6iqfy+a
qYA37p2li2qAxu1hgazRnvxYMuOo41Nl0U0bzIaAdGlwvq0RlVZHQgoquWH3C8e6
8B3dSHEVGmlN1tQtd9NoTWkW1FHLFa/2uoPhNEUWyWurk2MgLy1f30WGw/Hq4heQ
EMf1coK0Pf8ulwMMQsIptmb+b2Ja80JGVkKC7F+9VozYXD361ILHfkbUx8dSH5fZ
v1lhTrw7tEg1S4pZOu4AR+k7rGLOanV2MvEta+uWesd933eW6p06zhNV0/iZpSiC
SQOTchelyRY9v9BsajQx1qKKFXhv6wzyKI5A1/WqX4stwb/mpk1UW11NPvQ4Fns9
TUbEu4iiKcwjLVISzER5vQc+tmrT1wUk+fmTekXl/lrImez2uivsnKlUwq6r/2RD
GC8Ww3JeoeLkgV90peAtrMo83IiTNs2qHS/oqVUPQLt8FStXbPLeOg3unPcxHfQB
pwL3MhSdnXgOSI+wLnqLtCMVZFj9T5V3Hz8WK688zZImeopC12aCYRh7SRi19UVX
xZEtMlMFQp1Tgx+uMsxDYbub5fpS2LPLDfx07Rk9GzUwiqyWtspbZTZ/33f+TGYc
jh3ooWClo5yo7BC7VUzXc/w01sJvPx2SFkyeF//rcwVcKkdSJeDUwVKbs9CPUI5f
dXZTyJNpb70=
=Q7WT
-----END PGP SIGNATURE-----

--Sig_/YsmFTzy/q09G/2kW.vZ1AHi--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/