On 02. 01. 19, 16:04, Tetsuo Handa wrote:
> + if (wait_event_interruptible(tty->read_wait,
> + (ret = -EIO, test_bit(TTY_OTHER_CLOSED, &tty->flags)) ||
> + (ret = 0, tty_hung_up_p(file)) ||
> + (rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list)) != NULL ||
> + (ret = -EAGAIN, tty_io_nonblock(tty, file))))
> + return -EINTR;
Oh, that looks really ugly. Could you move all this to a function
returning a bool and taking &ret and &rbuf as parameters?
> + if (rbuf) {
> + if (rbuf->count > nr)
> + /* too large for caller's buffer */
> + ret = -EOVERFLOW;
> + else if (copy_to_user(buf, rbuf->buf, rbuf->count))
> + ret = -EFAULT;
> + else
> + ret = rbuf->count;
> + if (n_hdlc->rx_free_buf_list.count > DEFAULT_RX_BUF_COUNT)
> + kfree(rbuf);
> + else
> + n_hdlc_buf_put(&n_hdlc->rx_free_buf_list, rbuf);
> }
> -
> - remove_wait_queue(&tty->read_wait, &wait);
> - __set_current_state(TASK_RUNNING);
> -
> return ret;
> -
> } /* end of n_hdlc_tty_read() */
>
> /**
> @@ -645,21 +612,13 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty,
> struct file *file,
> static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
> const unsigned char *data, size_t count)
> {
> - struct n_hdlc *n_hdlc = tty2n_hdlc (tty);
> + struct n_hdlc *n_hdlc;
> int error = 0;
> - DECLARE_WAITQUEUE(wait, current);
> - struct n_hdlc_buf *tbuf;
> + struct n_hdlc_buf *tbuf = NULL;
>
> if (debuglevel >= DEBUG_LEVEL_INFO)
> printk("%s(%d)n_hdlc_tty_write() called count=%zd\n",
> __FILE__,__LINE__,count);
> -
> - /* Verify pointers */
> - if (!n_hdlc)
> - return -EIO;
> -
> - if (n_hdlc->magic != HDLC_MAGIC)
> - return -EIO;
>
> /* verify frame size */
> if (count > maxframe ) {
> @@ -670,40 +629,14 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty,
> struct file *file,
> maxframe );
> count = maxframe;
> }
> -
> - add_wait_queue(&tty->write_wait, &wait);
>
> - for (;;) {
> - set_current_state(TASK_INTERRUPTIBLE);
> -
> - tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list);
> - if (tbuf)
> - break;
> -
> - if (tty_io_nonblock(tty, file)) {
> - error = -EAGAIN;
> - break;
> - }
> - schedule();
> -
> - n_hdlc = tty2n_hdlc (tty);
> - if (!n_hdlc || n_hdlc->magic != HDLC_MAGIC ||
> - tty != n_hdlc->tty) {
> - printk("n_hdlc_tty_write: %p invalid after wait!\n",
> n_hdlc);
> - error = -EIO;
> - break;
> - }
> -
> - if (signal_pending(current)) {
> - error = -EINTR;
> - break;
> - }
> - }
> -
> - __set_current_state(TASK_RUNNING);
> - remove_wait_queue(&tty->write_wait, &wait);
> -
> - if (!error) {
> + if (wait_event_interruptible(tty->write_wait,
> + (error = -EIO, n_hdlc = tty2n_hdlc(tty), /* Verify pointers */
> + !n_hdlc || n_hdlc->magic != HDLC_MAGIC || tty != n_hdlc->tty) ||
> + (tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list)) != NULL ||
> + (error = -EAGAIN, tty_io_nonblock(tty, file))))
> + return -EINTR;
This is even worse. So detto as above?
> + if (tbuf) {
> /* Retrieve the user's buffer */
> memcpy(tbuf->buf, data, count);
>
> @@ -711,8 +644,9 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty,
> struct file *file,
> tbuf->count = error = count;
> n_hdlc_buf_put(&n_hdlc->tx_buf_list,tbuf);
> n_hdlc_send_frames(n_hdlc,tty);
> + } else if (error == -EIO) {
> + printk("n_hdlc_tty_write: %p invalid!\n", n_hdlc);
> }
> -
> return error;
>
> } /* end of n_hdlc_tty_write() */
>
thanks,
--
js
suse labs
