public inbox for cygwin-cvs@sourceware.org help / color / mirror / Atom feed
From: Corinna Vinschen <corinna@sourceware.org> To: cygwin-cvs@sourceware.org Subject: [newlib-cygwin] Cygwin: serial: read: revamp raw_read, change vmin_ and vtime_ to cc_t Date: Thu, 26 Mar 2020 11:26:16 +0000 (GMT) [thread overview] Message-ID: <20200326112616.91C03385E03B@sourceware.org> (raw) https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=93b491c4f25a622d1676c3f69f33d047f463bc2b commit 93b491c4f25a622d1676c3f69f33d047f463bc2b Author: Corinna Vinschen <corinna@vinschen.de> Date: Tue Mar 17 17:45:05 2020 +0100 Cygwin: serial: read: revamp raw_read, change vmin_ and vtime_ to cc_t - Datatypes were incorrect, especially vmin_ and vtime_. Change them to cc_t, as in user space. - Error checking had a gap or two. Debug output used the wrong formatting. - Don't use ev member for ClearCommError and WaitCommEvent. Both returned values are different (error value vs. event code). The values are not used elsewhere so it doesn't make sense to store them in the object. Therefore, drop ev member. - Some variable names were not very helpful. Especially using n as lpNumberOfBytesTransferred from GetOverlappedResult and then actually printing it as if it makes sense was quite puzzeling. - Rework the loop and the definition of minchars so that it still makes sense when looping. Signed-off-by: Corinna Vinschen <corinna@vinschen.de> Diff: --- winsup/cygwin/fhandler.h | 5 +- winsup/cygwin/fhandler_serial.cc | 206 ++++++++++++++++++++++----------------- 2 files changed, 117 insertions(+), 94 deletions(-) diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index d3afd753a..64a052ce1 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -1680,8 +1680,8 @@ class fhandler_cygdrive: public fhandler_disk_file class fhandler_serial: public fhandler_base { private: - size_t vmin_; /* from termios */ - unsigned int vtime_; /* from termios */ + cc_t vmin_; /* from termios */ + cc_t vtime_; /* from termios */ pid_t pgrp_; int rts; /* for Windows 9x purposes only */ int dtr; /* for Windows 9x purposes only */ @@ -1689,7 +1689,6 @@ class fhandler_serial: public fhandler_base public: int overlapped_armed; OVERLAPPED io_status; - DWORD ev; /* Constructor */ fhandler_serial (); diff --git a/winsup/cygwin/fhandler_serial.cc b/winsup/cygwin/fhandler_serial.cc index f729765e0..7492470f5 100644 --- a/winsup/cygwin/fhandler_serial.cc +++ b/winsup/cygwin/fhandler_serial.cc @@ -41,12 +41,21 @@ fhandler_serial::overlapped_setup () void __reg3 fhandler_serial::raw_read (void *ptr, size_t& ulen) { - int tot; - DWORD n; + DWORD io_err, event; + COMSTAT st; + DWORD bytes_to_read, read_bytes, undefined; + ssize_t tot = 0; + + if (ulen > SSIZE_MAX) + ulen = SSIZE_MAX; + if (ulen == 0) + return; - size_t minchars = vmin_ ? MIN (vmin_, ulen) : ulen; + /* If VMIN > 0 in blocking mode, we have to read at least VMIN chars, + otherwise we're in polling mode and there's no minimum chars. */ + ssize_t minchars = (!is_nonblocking () && vmin_) ? MIN (vmin_, ulen) : 0; - debug_printf ("ulen %ld, vmin_ %ld, vtime_ %u, hEvent %p", + debug_printf ("ulen %ld, vmin_ %u, vtime_ %u, hEvent %p", ulen, vmin_, vtime_, io_status.hEvent); if (!overlapped_armed) { @@ -54,122 +63,137 @@ fhandler_serial::raw_read (void *ptr, size_t& ulen) ResetEvent (io_status.hEvent); } - for (n = 0, tot = 0; ulen; ulen -= n, ptr = (char *) ptr + n) + do { - COMSTAT st; - DWORD inq = vmin_ ? minchars : vtime_ ? ulen : 1; - - n = 0; - - if (vtime_) // non-interruptible -- have to use kernel timeouts - overlapped_armed = -1; - - if (!ClearCommError (get_handle (), &ev, &st)) + /* First check if chars are already in the inbound queue. */ + if (!ClearCommError (get_handle (), &io_err, &st)) goto err; - else if (ev) - termios_printf ("error detected %x", ev); - else if (is_nonblocking ()) + /* FIXME: In case of I/O error, do we really want to bail out or is it + better just trying to pull through? */ + if (io_err) { - if (!st.cbInQue) - { - tot = -1; - set_errno (EAGAIN); - goto out; - } - inq = st.cbInQue; + termios_printf ("error detected %x", io_err); + SetLastError (ERROR_IO_DEVICE); + goto err; } - else if (st.cbInQue && !vtime_) - inq = st.cbInQue; - else if (!is_nonblocking () && !overlapped_armed) + /* ReadFile only handles up to DWORD bytes. */ + bytes_to_read = MIN (ulen, UINT32_MAX); + if (is_nonblocking ()) { - if ((size_t) tot >= minchars) + /* In O_NONBLOCK mode we just care for the number of chars already + in the inbound queue. */ + if (!st.cbInQue) break; - else if (WaitCommEvent (get_handle (), &ev, &io_status)) + bytes_to_read = MIN (st.cbInQue, bytes_to_read); + } + else + { + /* If the number of chars in the inbound queue is sufficent + (minchars defines the minimum), set bytes_to_read accordingly + and don't wait. */ + if (st.cbInQue && st.cbInQue >= minchars) + bytes_to_read = MIN (st.cbInQue, bytes_to_read); + /* if vtime_ is set, use kernel timeouts, otherwise wait here. */ + else if (vtime_ == 0 && !overlapped_armed) { - debug_printf ("WaitCommEvent succeeded: ev %x", ev); - if (!ev) - continue; + if (WaitCommEvent (get_handle (), &event, &io_status)) + { + debug_printf ("WaitCommEvent succeeded: event %x", event); + if (!event) + continue; + } + else if (GetLastError () != ERROR_IO_PENDING) + goto err; + overlapped_armed = 1; } - else if (GetLastError () != ERROR_IO_PENDING) - goto err; - else + } + /* overlapped_armed may be set by select, so we have to make sure + to disarm even in O_NONBLOCK mode. */ + if (overlapped_armed) + { + switch (cygwait (io_status.hEvent, is_nonblocking () ? 0 : INFINITE)) { - overlapped_armed = 1; - switch (cygwait (io_status.hEvent)) + case WAIT_OBJECT_0: + if (!GetOverlappedResult (get_handle (), &io_status, + &undefined, TRUE)) + goto err; + debug_printf ("overlapped event %x", event); + break; + case WAIT_SIGNALED: + CancelIo (get_handle ()); + overlapped_armed = 0; + if (!GetOverlappedResult (get_handle (), &io_status, + &undefined, TRUE)) + goto err; + /* Only if no bytes read, return with EINTR. */ + if (!tot) { - case WAIT_OBJECT_0: - if (!GetOverlappedResult (get_handle (), &io_status, &n, - FALSE)) - goto err; - debug_printf ("n %u, ev %x", n, ev); - break; - case WAIT_SIGNALED: tot = -1; - PurgeComm (get_handle (), PURGE_RXABORT); - overlapped_armed = 0; set_sig_errno (EINTR); - goto out; - case WAIT_CANCELED: - PurgeComm (get_handle (), PURGE_RXABORT); - overlapped_armed = 0; - pthread::static_cancel_self (); - /*NOTREACHED*/ - default: - goto err; } + goto out; + case WAIT_CANCELED: + CancelIo (get_handle ()); + overlapped_armed = 0; + GetOverlappedResult (get_handle (), &io_status, &undefined, + TRUE); + pthread::static_cancel_self (); + /*NOTREACHED*/ + default: + goto err; } } overlapped_armed = 0; ResetEvent (io_status.hEvent); - if (inq > ulen) - inq = ulen; - debug_printf ("inq %u", inq); - if (ReadFile (get_handle (), ptr, inq, &n, &io_status)) - /* Got something */; - else if (GetLastError () != ERROR_IO_PENDING) - goto err; - else if (is_nonblocking ()) + if (!ReadFile (get_handle (), ptr, bytes_to_read, &read_bytes, + &io_status)) { - /* Use CancelIo rather than PurgeComm (PURGE_RXABORT) since - PurgeComm apparently discards in-flight bytes while CancelIo - only stops the overlapped IO routine. */ - CancelIo (get_handle ()); - if (GetOverlappedResult (get_handle (), &io_status, &n, TRUE)) - tot = n; - else if (GetLastError () != ERROR_OPERATION_ABORTED) + if (GetLastError () != ERROR_IO_PENDING) + goto err; + if (is_nonblocking ()) + CancelIo (get_handle ()); + if (!GetOverlappedResult (get_handle (), &io_status, &read_bytes, + TRUE)) goto err; - if (tot == 0) - { - tot = -1; - set_errno (EAGAIN); - } - goto out; } - else if (!GetOverlappedResult (get_handle (), &io_status, &n, TRUE)) - goto err; - - tot += n; - debug_printf ("vtime_ %u, vmin_ %lu, n %u, tot %d", vtime_, vmin_, n, tot); - if (vtime_ || !vmin_ || !n) - break; + tot += read_bytes; + ptr = (void *) ((caddr_t) ptr + read_bytes); + ulen -= read_bytes; + minchars -= read_bytes; + debug_printf ("vtime_ %u, vmin_ %u, read_bytes %u, tot %D", + vtime_, vmin_, read_bytes, tot); continue; err: debug_printf ("err %E"); if (GetLastError () != ERROR_OPERATION_ABORTED) { - PurgeComm (get_handle (), PURGE_RXABORT); - tot = -1; - __seterrno (); + if (tot == 0) + { + tot = -1; + __seterrno (); + } + CancelIo (get_handle ()); + overlapped_armed = 0; + GetOverlappedResult (get_handle (), &io_status, &undefined, TRUE); break; } - - n = 0; } + /* ALL of these are required to loop: + + Still room in user space buffer + AND still a minchars requirement (implies blocking mode) + AND vtime_ is not set. */ + while (ulen > 0 && minchars > 0 && vtime_ == 0); out: - ulen = tot; + ulen = (size_t) tot; + if (is_nonblocking () && ulen == 0) + { + ulen = (size_t) -1; + set_errno (EAGAIN); + } } /* Cover function to WriteFile to provide Posix interface and semantics @@ -595,7 +619,7 @@ fhandler_serial::tcsetattr (int action, const struct termios *t) bool dropDTR = false; COMMTIMEOUTS to; DCB ostate, state; - unsigned int ovtime = vtime_, ovmin = vmin_; + cc_t ovtime = vtime_, ovmin = vmin_; int tmpDtr, tmpRts, res; res = tmpDtr = tmpRts = 0; @@ -902,7 +926,7 @@ fhandler_serial::tcsetattr (int action, const struct termios *t) vmin_ = t->c_cc[VMIN]; } - debug_printf ("vtime %d, vmin %ld", vtime_, vmin_); + debug_printf ("vtime %u, vmin %u", vtime_, vmin_); if (ovmin != vmin_ || ovtime != vtime_) { @@ -1136,7 +1160,7 @@ fhandler_serial::tcgetattr (struct termios *t) t->c_cc[VTIME] = vtime_ / 100; t->c_cc[VMIN] = vmin_; - debug_printf ("vmin_ %lu, vtime_ %u", vmin_, vtime_); + debug_printf ("vmin_ %u, vtime_ %u", vmin_, vtime_); return 0; }
reply other threads:[~2020-03-26 11:26 UTC|newest] Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200326112616.91C03385E03B@sourceware.org \ --to=corinna@sourceware.org \ --cc=cygwin-cvs@sourceware.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).