[PATCH 0/3] rbd: minor cleanups in rbd_add()

[PATCH 0/3] rbd: minor cleanups in rbd_add()

[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

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

[Alex Elder]

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 <elder@dreamhost.com>
---
  drivers/block/rbd.c |   32 ++++++++++++--------------------
  1 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index e7d6dfc..d5b4500 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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;
-- 
1.7.5.4

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

[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 <elder@dreamhost.com>
---
  drivers/block/rbd.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index d5b4500..1488a52 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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;

-- 
1.7.5.4

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

[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 <elder@dreamhost.com>
---
  drivers/block/rbd.c |   40 +++++++++++++++++-----------------------
  1 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 1488a52..29bbac1 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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)
-- 
1.7.5.4

Re: [PATCH 0/3] rbd: minor cleanups in rbd_add()

[Sage Weil]

Also good

On Tue, 28 Feb 2012, Alex Elder wrote:

> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>