[3/3] tty: Goto err when fail to unlock master pty

Submitted by Radostin Stoyanov on Nov. 16, 2018, 9:28 p.m.

Details

Message ID 20181116212853.3371-3-rstoyanov1@gmail.com
State Accepted
Series "Series without cover letter"
Headers show

Commit Message

Radostin Stoyanov Nov. 16, 2018, 9:28 p.m.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
---
 criu/tty.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/tty.c b/criu/tty.c
index 38e1cab3..11acfc18 100644
--- a/criu/tty.c
+++ b/criu/tty.c
@@ -997,7 +997,8 @@  static int pty_open_unpaired_slave(struct file_desc *d, struct tty_info *slave)
 				goto err;
 			}
 
-			unlock_pty(master);
+			if (unlock_pty(master))
+				goto err;
 
 			if (opts.orphan_pts_master &&
 			    rpc_send_fd(ACT_ORPHAN_PTS_MASTER, master) == 0) {
@@ -1036,7 +1037,8 @@  static int pty_open_unpaired_slave(struct file_desc *d, struct tty_info *slave)
 			goto err;
 		}
 
-		unlock_pty(master);
+		if (unlock_pty(master))
+			goto err;
 
 		fd = open_tty_reg(slave->reg_d, slave->tfe->flags);
 		if (fd < 0) {
@@ -1098,7 +1100,8 @@  static int pty_open_ptmx(struct tty_info *info)
 		return -1;
 	}
 
-	unlock_pty(master);
+	if (unlock_pty(master))
+		goto err;
 
 	if (restore_tty_params(master, info))
 		goto err;

Comments

Cyrill Gorcunov Nov. 16, 2018, 9:57 p.m.
On Fri, Nov 16, 2018 at 09:28:53PM +0000, Radostin Stoyanov wrote:
> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>

Not needed. If unlock failed then futher operations on locked
entry simply fails. (unlock itself will yield an error message
so we will notice)
Radostin Stoyanov Nov. 17, 2018, 12:11 a.m.
On 16/11/2018 21:57, Cyrill Gorcunov wrote:
> On Fri, Nov 16, 2018 at 09:28:53PM +0000, Radostin Stoyanov wrote:
>> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> Not needed. If unlock failed then futher operations on locked
> entry simply fails. (unlock itself will yield an error message
> so we will notice)
Thank you for the code review.

In this case should we make "static void unlock_pty"?
Cyrill Gorcunov Nov. 17, 2018, 7:21 a.m.
On Sat, Nov 17, 2018 at 12:11:53AM +0000, Radostin Stoyanov wrote:
> On 16/11/2018 21:57, Cyrill Gorcunov wrote:
> > On Fri, Nov 16, 2018 at 09:28:53PM +0000, Radostin Stoyanov wrote:
> >> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> > Not needed. If unlock failed then futher operations on locked
> > entry simply fails. (unlock itself will yield an error message
> > so we will notice)
> Thank you for the code review.
> 
> In this case should we make "static void unlock_pty"?
I think we could
Andrei Vagin Jan. 13, 2019, 5:18 a.m.
On Sat, Nov 17, 2018 at 12:57:28AM +0300, Cyrill Gorcunov wrote:
> On Fri, Nov 16, 2018 at 09:28:53PM +0000, Radostin Stoyanov wrote:
> > Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> 
> Not needed. If unlock failed then futher operations on locked
> entry simply fails. (unlock itself will yield an error message
> so we will notice)

I don't understand this. Why do we need to do "futher operations" in
this case?

> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Cyrill Gorcunov Jan. 13, 2019, 7:10 a.m.
On Sat, Jan 12, 2019 at 09:18:01PM -0800, Andrei Vagin wrote:
> On Sat, Nov 17, 2018 at 12:57:28AM +0300, Cyrill Gorcunov wrote:
> > On Fri, Nov 16, 2018 at 09:28:53PM +0000, Radostin Stoyanov wrote:
> > > Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> > 
> > Not needed. If unlock failed then futher operations on locked
> > entry simply fails. (unlock itself will yield an error message
> > so we will notice)
> 
> I don't understand this. Why do we need to do "futher operations" in
> this case?

Because pty pair is created in locked state, on which you can't
do anything, iow it is a first stage of pty restore. Later we
setup various parameters on the peer, thus we need to unlock
it first. In case if this action failed we will exit with error
anytway. The whole series is about code simplification and
shrinking.
Andrei Vagin Jan. 15, 2019, 5:43 p.m.
On Sun, Jan 13, 2019 at 10:10:18AM +0300, Cyrill Gorcunov wrote:
> On Sat, Jan 12, 2019 at 09:18:01PM -0800, Andrei Vagin wrote:
> > On Sat, Nov 17, 2018 at 12:57:28AM +0300, Cyrill Gorcunov wrote:
> > > On Fri, Nov 16, 2018 at 09:28:53PM +0000, Radostin Stoyanov wrote:
> > > > Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> > > 
> > > Not needed. If unlock failed then futher operations on locked
> > > entry simply fails. (unlock itself will yield an error message
> > > so we will notice)
> > 
> > I don't understand this. Why do we need to do "futher operations" in
> > this case?
> 
> Because pty pair is created in locked state, on which you can't
> do anything, iow it is a first stage of pty restore. Later we
> setup various parameters on the peer, thus we need to unlock
> it first. In case if this action failed we will exit with error
> anytway. The whole series is about code simplification and
> shrinking.

I still don't understand why we should not handle this error...

"code simplification and shrinking" doesn't mean that we don't want to
handle errors...
Cyrill Gorcunov Jan. 17, 2019, 9:03 a.m.
On Tue, Jan 15, 2019 at 09:43:14AM -0800, Andrei Vagin wrote:
> > Because pty pair is created in locked state, on which you can't
> > do anything, iow it is a first stage of pty restore. Later we
> > setup various parameters on the peer, thus we need to unlock
> > it first. In case if this action failed we will exit with error
> > anytway. The whole series is about code simplification and
> > shrinking.
> 
> I still don't understand why we should not handle this error...
> 
> "code simplification and shrinking" doesn't mean that we don't want to
> handle errors...

Well, if you still prefer to handle it explicitly then I think
we should simply drop this patch and leave the code as is.