Skip to contentSkip to conversation

rbd: minor cleanups in rbd_add()

PATCH In review Alex Elder 2 participants 5 messages Feb 29, 2012 – Mar 2, 2012

Subsystems (2)
Block layer RADOS block device (RBD)
lore
Share & tools
lore

Alex Elder

author
#0 lore raw

cover rbd: minor cleanups in rbd_add()

This series affects rbd_add(). It calls rbd_get_client(), and by making that function return a client pointer it makes it more obvious that the client pointer is getting assigned. It also reduces the amount of memory allocated to hold the monitor address and options passed from the user. And it cleans up the error handling path in that function.

-Alex

author
#1 lore raw

PATCH 1/3 rbd: have rbd_get_client() return a rbd_client

Since rbd_get_client() currently returns an error code. It assigns the rbd_client field of the rbd_device structure it is passed if successful. Instead, have it return the created rbd_client structure and return a pointer-coded error if there is an error. This makes the assignment of the client pointer more obvious at the call site.

Signed-off-by Alex Elder <[email protected]>
---
 drivers/block/rbd.c |   32 ++++++++++++--------------------
 1 files changed, 12 insertions(+), 20 deletions(-)
drivers/block/rbd.c +12 -20
index e7d6dfc..d5b4500 100644
@@ -384,17 +384,15 @@ static int parse_rbd_opts_token(char *c, void *private)
  * Get a ceph client with specific addr and configuration, if one does
  * not exist create it.
  */
-static int rbd_get_client(struct rbd_device *rbd_dev, const char *mon_addr,
-			  char *options)
+static struct rbd_client *rbd_get_client(const char *mon_addr, char *options)
 {
 	struct rbd_client *rbdc;
 	struct ceph_options *opt;
-	int ret;
 	struct rbd_options *rbd_opts;
 
 	rbd_opts = kzalloc(sizeof(*rbd_opts), GFP_KERNEL);
 	if (!rbd_opts)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	rbd_opts->notify_timeout = RBD_NOTIFY_TIMEOUT_DEFAULT;
 

@@ -402,8 +400,8 @@ static int rbd_get_client(struct rbd_device *rbd_dev, const char *mon_addr,
 				mon_addr + strlen(mon_addr),
 				parse_rbd_opts_token, rbd_opts);
 	if (IS_ERR(opt)) {
-		ret = PTR_ERR(opt);
-		goto done_err;
+		kfree(rbd_opts);
+		return ERR_CAST(opt);
 	}
 
 	spin_lock(&rbd_client_list_lock);

@@ -413,27 +411,19 @@ static int rbd_get_client(struct rbd_device *rbd_dev, const char *mon_addr,
 		kref_get(&rbdc->kref);
 		spin_unlock(&rbd_client_list_lock);
 
-		rbd_dev->rbd_client = rbdc;
-
 		ceph_destroy_options(opt);
 		kfree(rbd_opts);
 
-		return 0;
+		return rbdc;
 	}
 	spin_unlock(&rbd_client_list_lock);
 
 	rbdc = rbd_client_create(opt, rbd_opts);
 
-	if (IS_ERR(rbdc)) {
-		ret = PTR_ERR(rbdc);
-		goto done_err;
-	}
+	if (IS_ERR(rbdc))
+		kfree(rbd_opts);
 
-	rbd_dev->rbd_client = rbdc;
-	return 0;
-done_err:
-	kfree(rbd_opts);
-	return ret;
+	return rbdc;
 }
 
 /*

@@ -2287,9 +2277,11 @@ static ssize_t rbd_add(struct bus_type *bus,
 	/* initialize rest of new object */
 	snprintf(rbd_dev->name, DEV_NAME_LEN, RBD_DRV_NAME "%d", rbd_dev->id);
 
-	rc = rbd_get_client(rbd_dev, mon_dev_name, options);
-	if (rc < 0)
+	rbd_dev->rbd_client = rbd_get_client(mon_dev_name, options);
+	if (IS_ERR(rbd_dev->rbd_client)) {
+		rc = PTR_ERR(rbd_dev->rbd_client);
 		goto err_put_id;
+	}
 
 	/* pick the pool */
 	osdc = &rbd_dev->rbd_client->client->osdc;
Signature · 1 line
1.7.5.4
author
#2 lore raw

PATCH 2/3 rbd: reduce memory used for rbd_dev fields

In reply to Alex Elder

The length of the string containing the monitor address specification(s) will never exceed the length of the string passed in to rbd_add(). The same holds true for the ceph + rbd options string. So reduce the amount of memory allocated for these to that length rather than the maximum (1024 bytes).

Signed-off-by Alex Elder <[email protected]>
---
 drivers/block/rbd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/block/rbd.c +2 -2
index d5b4500..1488a52 100644
@@ -2233,11 +2233,11 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;
 
-	mon_dev_name = kmalloc(RBD_MAX_OPT_LEN, GFP_KERNEL);
+	mon_dev_name = kmalloc(count, GFP_KERNEL);
 	if (!mon_dev_name)
 		goto err_out_mod;
 
-	options = kmalloc(RBD_MAX_OPT_LEN, GFP_KERNEL);
+	options = kmalloc(count, GFP_KERNEL);
 	if (!options)
 		goto err_mon_dev;
Signature · 1 line
1.7.5.4
author
#3 lore raw

PATCH 3/3 rbd: simplify error handling in rbd_add()

In reply to Alex Elder

If a couple pointers are initialized to NULL then a single "out_nomem" label can be used for all of the memory allocation failure cases in rbd_add().

Also, get rid of the "irc" local variable there. There is no real need for "rc" to be type ssize_t, and it can be used in the spot "irc" was.

Signed-off-by Alex Elder <[email protected]>
---
 drivers/block/rbd.c |   40 +++++++++++++++++-----------------------
 1 files changed, 17 insertions(+), 23 deletions(-)
drivers/block/rbd.c +17 -23
index 1488a52..29bbac1 100644
@@ -2223,28 +2223,24 @@ static ssize_t rbd_add(struct bus_type *bus,
 		       const char *buf,
 		       size_t count)
 {
-	struct ceph_osd_client *osdc;
 	struct rbd_device *rbd_dev;
-	ssize_t rc = -ENOMEM;
-	int irc;
-	char *mon_dev_name;
-	char *options;
+	char *mon_dev_name = NULL;
+	char *options = NULL;
+	struct ceph_osd_client *osdc;
+	int rc = -ENOMEM;
 
 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;
 
+	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
+	if (!rbd_dev)
+		goto err_nomem;
 	mon_dev_name = kmalloc(count, GFP_KERNEL);
 	if (!mon_dev_name)
-		goto err_out_mod;
-
+		goto err_nomem;
 	options = kmalloc(count, GFP_KERNEL);
 	if (!options)
-		goto err_mon_dev;
-
-	/* new rbd_device object */
-	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
-	if (!rbd_dev)
-		goto err_out_opt;
+		goto err_nomem;
 
 	/* static rbd_device initialization */
 	spin_lock_init(&rbd_dev->lock);

@@ -2291,12 +2287,10 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rbd_dev->poolid = rc;
 
 	/* register our block device */
-	irc = register_blkdev(0, rbd_dev->name);
-	if (irc < 0) {
-		rc = irc;
+	rc = register_blkdev(0, rbd_dev->name);
+	if (rc < 0)
 		goto err_out_client;
-	}
-	rbd_dev->major = irc;
+	rbd_dev->major = rc;
 
 	rc = rbd_bus_add_dev(rbd_dev);
 	if (rc)

@@ -2329,15 +2323,15 @@ err_out_client:
 	rbd_put_client(rbd_dev);
 err_put_id:
 	rbd_id_put(rbd_dev);
-	kfree(rbd_dev);
-err_out_opt:
+err_nomem:
 	kfree(options);
-err_mon_dev:
 	kfree(mon_dev_name);
-err_out_mod:
+	kfree(rbd_dev);
+
 	dout("Error adding device %s\n", buf);
 	module_put(THIS_MODULE);
-	return rc;
+
+	return (ssize_t) rc;
 }
 
 static struct rbd_device *__rbd_get_dev(unsigned long id)
Signature · 1 line
1.7.5.4
#4 lore raw
In reply to Alex Elder

Also good

Quoted 14 lines · Alex Elder

This series affects rbd_add(). It calls rbd_get_client(), and by making that function return a client pointer it makes it more obvious that the client pointer is getting assigned. It also reduces the amount of memory allocated to hold the monitor address and options passed from the user. And it cleans up the error handling path in that function.

-Alex
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html