[1/2] Revert "9p: p9_client_create: use p9_client_destroy on failure"

Commit Message

Leon Romanovsky Sept. 29, 2022, 9:37 a.m. UTC

Rely on proper unwind order.

This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.

Reported-by: [e-mail]
Signed-off-by: Leon Romanovsky <[e-mail]>
---
 net/9p/client.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Dan Carpenter Oct. 4, 2022, 1:10 p.m. UTC | #1

On Thu, Sep 29, 2022 at 12:37:55PM +0300, Leon Romanovsky wrote:
> Rely on proper unwind order.
> 
> This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.
> 
> Reported-by: [e-mail]
> Signed-off-by: Leon Romanovsky <[e-mail]>

The commit message doesn't really say what the problem is to the user.
Is this just to make the next patch easier?

regards,
dan carpenter

Dominique Martinet Oct. 4, 2022, 9:01 p.m. UTC | #2

Dan Carpenter wrote on Tue, Oct 04, 2022 at 04:10:44PM +0300:
> On Thu, Sep 29, 2022 at 12:37:55PM +0300, Leon Romanovsky wrote:
> > Rely on proper unwind order.
> > 
> > This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.
> > 
> > Reported-by: [e-mail]
> > Signed-off-by: Leon Romanovsky <[e-mail]>
> 
> The commit message doesn't really say what the problem is to the user.
> Is this just to make the next patch easier?

Yes (and perhaps a bit of spite from the previous discussion), and the
next patch was not useable so I am not applying this as is.

The next patch was meant as an alternative implementation to this fix:
https://lore.kernel.org/all/[e-mail]/T/#u

At this point I have the original fix in my -next branch but it hasn't
had any positive review (and well, I myself agree it's ugly), so unless
Leon sends a v2 I'll need to think of a better way of tracking if
clnt->trans_mod->create has been successfully called.
I'm starting to think that since we don't have so many clnt I can
probably just fit a bool/bitfield in one of the holes of the struct
and just keep track of it; that'll be less error-prone than relying on
clnt->trans (which -is- initialized in create() most of the time, but
not in a way we can rely on) or reworking create() to return it as I
originally wanted to do (rdma still needs to populate clnt->trans behind
the scenees before create() returns, so the abstraction is also quite
ugly)

The current breakage is actually quite bad so I'll try to send that
today or tomorrow for merging next week unless Leon resends something we
can work with... Conceptually won't be different than the patch
currently in next so hopefully can pretend it's had a couple of weeks of
testing...

Leon Romanovsky Oct. 6, 2022, 4:19 p.m. UTC | #3

On Wed, Oct 05, 2022 at 06:01:39AM +0900, Dominique Martinet wrote:
> Dan Carpenter wrote on Tue, Oct 04, 2022 at 04:10:44PM +0300:
> > On Thu, Sep 29, 2022 at 12:37:55PM +0300, Leon Romanovsky wrote:
> > > Rely on proper unwind order.
> > > 
> > > This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.
> > > 
> > > Reported-by: [e-mail]
> > > Signed-off-by: Leon Romanovsky <[e-mail]>
> > 
> > The commit message doesn't really say what the problem is to the user.
> > Is this just to make the next patch easier?
> 
> Yes (and perhaps a bit of spite from the previous discussion), and the
> next patch was not useable so I am not applying this as is.
> 
> The next patch was meant as an alternative implementation to this fix:
> https://lore.kernel.org/all/[e-mail]/T/#u
> 
> At this point I have the original fix in my -next branch but it hasn't
> had any positive review (and well, I myself agree it's ugly), so unless
> Leon sends a v2 I'll need to think of a better way of tracking if
> clnt->trans_mod->create has been successfully called.
> I'm starting to think that since we don't have so many clnt I can
> probably just fit a bool/bitfield in one of the holes of the struct
> and just keep track of it; that'll be less error-prone than relying on
> clnt->trans (which -is- initialized in create() most of the time, but
> not in a way we can rely on) or reworking create() to return it as I
> originally wanted to do (rdma still needs to populate clnt->trans behind
> the scenees before create() returns, so the abstraction is also quite
> ugly)
> 
> The current breakage is actually quite bad so I'll try to send that
> today or tomorrow for merging next week unless Leon resends something we
> can work with... Conceptually won't be different than the patch
> currently in next so hopefully can pretend it's had a couple of weeks of
> testing...

I can't resend anything in near future and most of the time have very
limited access to computer due to holidays here.

Thanks

> -- 
> Dominique

Patch

diff --git a/net/9p/client.c b/net/9p/client.c
index bfa80f01992e..aaa37b07e30a 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -961,10 +961,14 @@  struct p9_client *p9_client_create(const char *dev_name, char *options)
 	char *client_id;
 
 	err = 0;
-	clnt = kzalloc(sizeof(*clnt), GFP_KERNEL);
+	clnt = kmalloc(sizeof(*clnt), GFP_KERNEL);
 	if (!clnt)
 		return ERR_PTR(-ENOMEM);
 
+	clnt->trans_mod = NULL;
+	clnt->trans = NULL;
+	clnt->fcall_cache = NULL;
+
 	client_id = utsname()->nodename;
 	memcpy(clnt->name, client_id, strlen(client_id) + 1);
 
@@ -974,7 +978,7 @@  struct p9_client *p9_client_create(const char *dev_name, char *options)
 
 	err = parse_opts(options, clnt);
 	if (err < 0)
-		goto out;
+		goto free_client;
 
 	if (!clnt->trans_mod)
 		clnt->trans_mod = v9fs_get_default_trans();
@@ -983,7 +987,7 @@  struct p9_client *p9_client_create(const char *dev_name, char *options)
 		err = -EPROTONOSUPPORT;
 		p9_debug(P9_DEBUG_ERROR,
 			 "No transport defined or default transport\n");
-		goto out;
+		goto free_client;
 	}
 
 	p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",
@@ -991,7 +995,7 @@  struct p9_client *p9_client_create(const char *dev_name, char *options)
 
 	err = clnt->trans_mod->create(clnt, dev_name, options);
 	if (err)
-		goto out;
+		goto put_trans;
 
 	if (clnt->msize > clnt->trans_mod->maxsize) {
 		clnt->msize = clnt->trans_mod->maxsize;
@@ -1005,12 +1009,12 @@  struct p9_client *p9_client_create(const char *dev_name, char *options)
 		p9_debug(P9_DEBUG_ERROR,
 			 "Please specify a msize of at least 4k\n");
 		err = -EINVAL;
-		goto out;
+		goto close_trans;
 	}
 
 	err = p9_client_version(clnt);
 	if (err)
-		goto out;
+		goto close_trans;
 
 	/* P9_HDRSZ + 4 is the smallest packet header we can have that is
 	 * followed by data accessed from userspace by read
@@ -1023,8 +1027,12 @@  struct p9_client *p9_client_create(const char *dev_name, char *options)
 
 	return clnt;
 
-out:
-	p9_client_destroy(clnt);
+close_trans:
+	clnt->trans_mod->close(clnt);
+put_trans:
+	v9fs_put_trans(clnt->trans_mod);
+free_client:
+	kfree(clnt);
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL(p9_client_create);