[Qemu-devel] [PATCH 1/3] rbd: fix leak in failure path

[Qemu-devel] [PATCH 1/3] rbd: fix leak in failure path

[Sage Weil]

Fix leak of s->snap when rados_create fails.

Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Sage Weil <sage@newdream.net>
---
 block/rbd.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index d5659cd..52b79fa 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -393,10 +393,6 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
                            conf, sizeof(conf)) < 0) {
         return -EINVAL;
     }
-    s->snap = NULL;
-    if (snap_buf[0] != '\0') {
-        s->snap = qemu_strdup(snap_buf);
-    }
 
     r = rados_create(&s->cluster, NULL);
     if (r < 0) {
@@ -404,6 +400,11 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
         return r;
     }
 
+    s->snap = NULL;
+    if (snap_buf[0] != '\0') {
+        s->snap = qemu_strdup(snap_buf);
+    }
+
     if (strstr(conf, "conf=") == NULL) {
         r = rados_conf_read_file(s->cluster, NULL);
         if (r < 0) {
-- 
1.7.0

[Qemu-devel] [PATCH 2/3] rbd: allow client id to be specified in config string

[Sage Weil]

Allow the client id to be specified in the config string via 'id=' so that
users can control who they authenticate as.  Currently they are stuck with
the default ('admin').  This is necessary for anyone using authentication
in their environment.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 block/rbd.c |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 52b79fa..e239e04 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -169,6 +169,34 @@ done:
     return ret;
 }
 
+static char *qemu_rbd_parse_clientname(const char *conf, char *clientname)
+{
+    const char *p = conf;
+
+    while (*p) {
+        int len;
+        const char *end = strchr(p, ':');
+
+        if (end) {
+            len = end - p;
+        } else {
+            len = strlen(p);
+        }
+
+        if (strncmp(p, "id=", 3) == 0) {
+            len -= 3;
+            strncpy(clientname, p + 3, len);
+            clientname[len] = '\0';
+            return clientname;
+        }
+        if (end == NULL) {
+            break;
+        }
+        p = end + 1;
+    }
+    return NULL;
+}
+
 static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
 {
     char *p, *buf;
@@ -198,17 +226,19 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
             break;
         }
 
-        if (strcmp(name, "conf")) {
-            ret = rados_conf_set(cluster, name, value);
+        if (strcmp(name, "conf") == 0) {
+            ret = rados_conf_read_file(cluster, value);
             if (ret < 0) {
-                error_report("invalid conf option %s", name);
-                ret = -EINVAL;
+                error_report("error reading conf file %s", value);
                 break;
             }
+        } else if (strcmp(name, "id") == 0) {
+            /* ignore, this is parsed by qemu_rbd_parse_clientname() */
         } else {
-            ret = rados_conf_read_file(cluster, value);
+            ret = rados_conf_set(cluster, name, value);
             if (ret < 0) {
-                error_report("error reading conf file %s", value);
+                error_report("invalid conf option %s", name);
+                ret = -EINVAL;
                 break;
             }
         }
@@ -227,6 +257,8 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
     char name[RBD_MAX_IMAGE_NAME_SIZE];
     char snap_buf[RBD_MAX_SNAP_NAME_SIZE];
     char conf[RBD_MAX_CONF_SIZE];
+    char clientname_buf[RBD_MAX_CONF_SIZE];
+    char *clientname;
     rados_t cluster;
     rados_ioctx_t io_ctx;
     int ret;
@@ -259,7 +291,8 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    if (rados_create(&cluster, NULL) < 0) {
+    clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
+    if (rados_create(&cluster, clientname) < 0) {
         error_report("error initializing");
         return -EIO;
     }
@@ -385,6 +418,8 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
     char pool[RBD_MAX_POOL_NAME_SIZE];
     char snap_buf[RBD_MAX_SNAP_NAME_SIZE];
     char conf[RBD_MAX_CONF_SIZE];
+    char clientname_buf[RBD_MAX_CONF_SIZE];
+    char *clientname;
     int r;
 
     if (qemu_rbd_parsename(filename, pool, sizeof(pool),
@@ -394,7 +429,8 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
         return -EINVAL;
     }
 
-    r = rados_create(&s->cluster, NULL);
+    clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
+    r = rados_create(&s->cluster, clientname);
     if (r < 0) {
         error_report("error initializing");
         return r;
-- 
1.7.0

[Qemu-devel] [PATCH 3/3] rbd: clean up, fix style

[Sage Weil]

No assignment in condition.  Remove duplicate ret > 0 check.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 block/rbd.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index e239e04..5423a2d 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -391,15 +391,14 @@ static void qemu_rbd_aio_event_reader(void *opaque)
         char *p = (char *)&s->event_rcb;
 
         /* now read the rcb pointer that was sent from a non qemu thread */
-        if ((ret = read(s->fds[RBD_FD_READ], p + s->event_reader_pos,
-                        sizeof(s->event_rcb) - s->event_reader_pos)) > 0) {
-            if (ret > 0) {
-                s->event_reader_pos += ret;
-                if (s->event_reader_pos == sizeof(s->event_rcb)) {
-                    s->event_reader_pos = 0;
-                    qemu_rbd_complete_aio(s->event_rcb);
-                    s->qemu_aio_count--;
-                }
+        ret = read(s->fds[RBD_FD_READ], p + s->event_reader_pos,
+                   sizeof(s->event_rcb) - s->event_reader_pos);
+        if (ret > 0) {
+            s->event_reader_pos += ret;
+            if (s->event_reader_pos == sizeof(s->event_rcb)) {
+                s->event_reader_pos = 0;
+                qemu_rbd_complete_aio(s->event_rcb);
+                s->qemu_aio_count--;
             }
         }
     } while (ret < 0 && errno == EINTR);
-- 
1.7.0

Re: [Qemu-devel] [PATCH 1/3] rbd: fix leak in failure path

[Stefan Hajnoczi]

On Tue, Aug 23, 2011 at 5:28 PM, Sage Weil <sage@newdream.net> wrote:
> Fix leak of s->snap when rados_create fails.
>
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  block/rbd.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index d5659cd..52b79fa 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -393,10 +393,6 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
>                            conf, sizeof(conf)) < 0) {
>         return -EINVAL;
>     }
> -    s->snap = NULL;
> -    if (snap_buf[0] != '\0') {
> -        s->snap = qemu_strdup(snap_buf);
> -    }
>
>     r = rados_create(&s->cluster, NULL);
>     if (r < 0) {
> @@ -404,6 +400,11 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
>         return r;
>     }
>
> +    s->snap = NULL;
> +    if (snap_buf[0] != '\0') {
> +        s->snap = qemu_strdup(snap_buf);
> +    }
> +
>     if (strstr(conf, "conf=") == NULL) {
>         r = rados_conf_read_file(s->cluster, NULL);
>         if (r < 0) {

I think s->snap is still leaked when any other error return in this
function is taken.

Stefan

Re: [Qemu-devel] [PATCH 2/3] rbd: allow client id to be specified in config string

[Stefan Hajnoczi]

On Tue, Aug 23, 2011 at 5:28 PM, Sage Weil <sage@newdream.net> wrote:
> Allow the client id to be specified in the config string via 'id=' so that
> users can control who they authenticate as.  Currently they are stuck with
> the default ('admin').  This is necessary for anyone using authentication
> in their environment.
>
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  block/rbd.c |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 44 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 3/3] rbd: clean up, fix style

[Stefan Hajnoczi]

On Tue, Aug 23, 2011 at 5:28 PM, Sage Weil <sage@newdream.net> wrote:
> No assignment in condition.  Remove duplicate ret > 0 check.
>
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  block/rbd.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] [PATCH v2] rbd: fix leak in qemu_rbd_open failure paths

[Sage Weil]

Fix leak of s->snap in failure path.  Simplify error paths for the whole
function.

Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Sage Weil <sage@newdream.net>
---
v2: fixes all error paths, not just the first one. 

 block/rbd.c |   28 +++++++++++++---------------
 1 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index eaf7912..2f0733e 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -427,10 +427,6 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
                            conf, sizeof(conf)) < 0) {
         return -EINVAL;
     }
-    s->snap = NULL;
-    if (snap_buf[0] != '\0') {
-        s->snap = qemu_strdup(snap_buf);
-    }
 
     clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
     r = rados_create(&s->cluster, clientname);
@@ -439,12 +435,16 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
         return r;
     }
 
+    s->snap = NULL;
+    if (snap_buf[0] != '\0') {
+        s->snap = qemu_strdup(snap_buf);
+    }
+
     if (strstr(conf, "conf=") == NULL) {
         r = rados_conf_read_file(s->cluster, NULL);
         if (r < 0) {
             error_report("error reading config file");
-            rados_shutdown(s->cluster);
-            return r;
+	    goto failed_shutdown;
         }
     }
 
@@ -452,31 +452,26 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
         r = qemu_rbd_set_conf(s->cluster, conf);
         if (r < 0) {
             error_report("error setting config options");
-            rados_shutdown(s->cluster);
-            return r;
+	    goto failed_shutdown;
         }
     }
 
     r = rados_connect(s->cluster);
     if (r < 0) {
         error_report("error connecting");
-        rados_shutdown(s->cluster);
-        return r;
+	goto failed_shutdown;
     }
 
     r = rados_ioctx_create(s->cluster, pool, &s->io_ctx);
     if (r < 0) {
         error_report("error opening pool %s", pool);
-        rados_shutdown(s->cluster);
-        return r;
+	goto failed_shutdown;
     }
 
     r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
     if (r < 0) {
         error_report("error reading header from %s", s->name);
-        rados_ioctx_destroy(s->io_ctx);
-        rados_shutdown(s->cluster);
-        return r;
+	goto failed_open;
     }
 
     bs->read_only = (s->snap != NULL);
@@ -497,8 +492,11 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
 
 failed:
     rbd_close(s->image);
+failed_open:
     rados_ioctx_destroy(s->io_ctx);
+failed_shutdown:
     rados_shutdown(s->cluster);
+    qemu_free(s->snap);
     return r;
 }
 
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v2] rbd: fix leak in qemu_rbd_open failure paths

[Stefan Hajnoczi]

On Sat, Sep 3, 2011 at 11:04 PM, Sage Weil <sage@newdream.net> wrote:
> +failed_shutdown:
>     rados_shutdown(s->cluster);
> +    qemu_free(s->snap);

Sorry for being a pain here.  This patch is against an old qemu.git
tree.  All memory allocation is now using glib's g_malloc()/g_free().

Stefan

Re: [Qemu-devel] [PATCH v2] rbd: fix leak in qemu_rbd_open failure paths

[Sage Weil]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 493 bytes --]

On Sun, 4 Sep 2011, Stefan Hajnoczi wrote:
> On Sat, Sep 3, 2011 at 11:04 PM, Sage Weil <sage@newdream.net> wrote:
> > +failed_shutdown:
> >     rados_shutdown(s->cluster);
> > +    qemu_free(s->snap);
> 
> Sorry for being a pain here.  This patch is against an old qemu.git
> tree.  All memory allocation is now using glib's g_malloc()/g_free().

Heh, no problem.  We've been working off the last release to avoid 
worrying about transient instability in master.  Resending!

sage

[Qemu-devel] [PATCH v3] rbd: fix leak in qemu_rbd_open failure paths

[Sage Weil]

Fix leak of s->snap in failure path.  Simplify error paths for the whole
function.

Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Sage Weil <sage@newdream.net>
---
 block/rbd.c |   28 +++++++++++++---------------
 1 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 2763092..9f32211 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -427,10 +427,6 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
                            conf, sizeof(conf)) < 0) {
         return -EINVAL;
     }
-    s->snap = NULL;
-    if (snap_buf[0] != '\0') {
-        s->snap = g_strdup(snap_buf);
-    }
 
     clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
     r = rados_create(&s->cluster, clientname);
@@ -439,12 +435,16 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
         return r;
     }
 
+    s->snap = NULL;
+    if (snap_buf[0] != '\0') {
+        s->snap = g_strdup(snap_buf);
+    }
+
     if (strstr(conf, "conf=") == NULL) {
         r = rados_conf_read_file(s->cluster, NULL);
         if (r < 0) {
             error_report("error reading config file");
-            rados_shutdown(s->cluster);
-            return r;
+	    goto failed_shutdown;
         }
     }
 
@@ -452,31 +452,26 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
         r = qemu_rbd_set_conf(s->cluster, conf);
         if (r < 0) {
             error_report("error setting config options");
-            rados_shutdown(s->cluster);
-            return r;
+	    goto failed_shutdown;
         }
     }
 
     r = rados_connect(s->cluster);
     if (r < 0) {
         error_report("error connecting");
-        rados_shutdown(s->cluster);
-        return r;
+	goto failed_shutdown;
     }
 
     r = rados_ioctx_create(s->cluster, pool, &s->io_ctx);
     if (r < 0) {
         error_report("error opening pool %s", pool);
-        rados_shutdown(s->cluster);
-        return r;
+	goto failed_shutdown;
     }
 
     r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
     if (r < 0) {
         error_report("error reading header from %s", s->name);
-        rados_ioctx_destroy(s->io_ctx);
-        rados_shutdown(s->cluster);
-        return r;
+	goto failed_open;
     }
 
     bs->read_only = (s->snap != NULL);
@@ -497,8 +492,11 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
 
 failed:
     rbd_close(s->image);
+failed_open:
     rados_ioctx_destroy(s->io_ctx);
+failed_shutdown:
     rados_shutdown(s->cluster);
+    g_free(s->snap);
     return r;
 }
 
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v3] rbd: fix leak in qemu_rbd_open failure paths

[Stefan Hajnoczi]

On Sun, Sep 4, 2011 at 5:19 PM, Sage Weil <sage@newdream.net> wrote:
> Fix leak of s->snap in failure path.  Simplify error paths for the whole
> function.
>
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  block/rbd.c |   28 +++++++++++++---------------
>  1 files changed, 13 insertions(+), 15 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH v3] rbd: fix leak in qemu_rbd_open failure paths

[Kevin Wolf]

Am 04.09.2011 18:19, schrieb Sage Weil:
> Fix leak of s->snap in failure path.  Simplify error paths for the whole
> function.
> 
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Sage Weil <sage@newdream.net>

This depends on "[PATCH v2] rbd: allow client id to be specified in
config string", which doesn't seem to apply any more and has coding
style issues. Please fix and send a rebased series containing all four
patches.

Kevin