[Qemu-devel] [PATCH 0/4] block/gluster: cleanups for GlfsPreopened

[Qemu-devel] [PATCH 0/4] block/gluster: cleanups for GlfsPreopened

[Stefan Hajnoczi]

Code added in commit 6349c15410361d3fe52c9beee309954d606f8ccd ("block/gluster:
memory usage: use one glfs instance per volume") does not follow conventions
and violates QEMU coding style.  Although any single issue in isolation is not
worth patching, there are several of these and I think it's worth resolving
them together in one sweep.

Stefan Hajnoczi (4):
  block/gluster: fix wrong indent in glfs_find_preopened()
  block/gluster: drop intermediate ListElement struct
  block/gluster: use conventional names for GlfsPreopened functions
  block/gluster: add missing QLIST_HEAD_INITIALIZER()

 block/gluster.c | 66 +++++++++++++++++++++++++++------------------------------
 1 file changed, 31 insertions(+), 35 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/4] block/gluster: fix wrong indent in glfs_find_preopened()

[Stefan Hajnoczi]

QEMU uses 4-space indentation.  Fix this now so checkpatch.pl is happy
with future code changes.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/gluster.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 1a22f29..516a1e1 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -226,12 +226,12 @@ static glfs_t *glfs_find_preopened(const char *volume)
 {
     ListElement *entry = NULL;
 
-     QLIST_FOREACH(entry, &glfs_list, list) {
+    QLIST_FOREACH(entry, &glfs_list, list) {
         if (strcmp(entry->saved.volume, volume) == 0) {
             entry->saved.ref++;
             return entry->saved.fs;
         }
-     }
+    }
 
     return NULL;
 }
-- 
2.9.3

[Qemu-devel] [PATCH 2/4] block/gluster: drop intermediate ListElement struct

[Stefan Hajnoczi]

The "qemu/queue.h" data structures are used without intermediate list
node structs.  They are designed to be embedded in the main struct.
Drop the unnecessary ListElement struct.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/gluster.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 516a1e1..171a323 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -56,19 +56,14 @@ typedef struct BDRVGlusterReopenState {
     struct glfs_fd *fd;
 } BDRVGlusterReopenState;
 
-
 typedef struct GlfsPreopened {
     char *volume;
     glfs_t *fs;
     int ref;
+    QLIST_ENTRY(GlfsPreopened) list;
 } GlfsPreopened;
 
-typedef struct ListElement {
-    QLIST_ENTRY(ListElement) list;
-    GlfsPreopened saved;
-} ListElement;
-
-static QLIST_HEAD(glfs_list, ListElement) glfs_list;
+static QLIST_HEAD(glfs_list, GlfsPreopened) glfs_list;
 
 static QemuOptsList qemu_gluster_create_opts = {
     .name = "qemu-gluster-create-opts",
@@ -210,26 +205,26 @@ static QemuOptsList runtime_tcp_opts = {
 
 static void glfs_set_preopened(const char *volume, glfs_t *fs)
 {
-    ListElement *entry = NULL;
+    GlfsPreopened *entry = NULL;
 
-    entry = g_new(ListElement, 1);
+    entry = g_new(GlfsPreopened, 1);
 
-    entry->saved.volume = g_strdup(volume);
+    entry->volume = g_strdup(volume);
 
-    entry->saved.fs = fs;
-    entry->saved.ref = 1;
+    entry->fs = fs;
+    entry->ref = 1;
 
     QLIST_INSERT_HEAD(&glfs_list, entry, list);
 }
 
 static glfs_t *glfs_find_preopened(const char *volume)
 {
-    ListElement *entry = NULL;
+    GlfsPreopened *entry = NULL;
 
     QLIST_FOREACH(entry, &glfs_list, list) {
-        if (strcmp(entry->saved.volume, volume) == 0) {
-            entry->saved.ref++;
-            return entry->saved.fs;
+        if (strcmp(entry->volume, volume) == 0) {
+            entry->ref++;
+            return entry->fs;
         }
     }
 
@@ -238,23 +233,23 @@ static glfs_t *glfs_find_preopened(const char *volume)
 
 static void glfs_clear_preopened(glfs_t *fs)
 {
-    ListElement *entry = NULL;
-    ListElement *next;
+    GlfsPreopened *entry = NULL;
+    GlfsPreopened *next;
 
     if (fs == NULL) {
         return;
     }
 
     QLIST_FOREACH_SAFE(entry, &glfs_list, list, next) {
-        if (entry->saved.fs == fs) {
-            if (--entry->saved.ref) {
+        if (entry->fs == fs) {
+            if (--entry->ref) {
                 return;
             }
 
             QLIST_REMOVE(entry, list);
 
-            glfs_fini(entry->saved.fs);
-            g_free(entry->saved.volume);
+            glfs_fini(entry->fs);
+            g_free(entry->volume);
             g_free(entry);
         }
     }
-- 
2.9.3

[Qemu-devel] [PATCH 3/4] block/gluster: use conventional names for GlfsPreopened functions

[Stefan Hajnoczi]

The naming of GlfsPreopened functions is a little unusual:

glfs_set_preopened() appends items to the list.  Normally this operation
is called "add".

glfs_find_preopened() is paired with glfs_clear_preopened().  Normally
this is called "get" and "put" (or "ref" and "unref").

This patch renames these functions.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/gluster.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 171a323..181b345 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -203,7 +203,7 @@ static QemuOptsList runtime_tcp_opts = {
     },
 };
 
-static void glfs_set_preopened(const char *volume, glfs_t *fs)
+static void glfs_add_preopened(const char *volume, glfs_t *fs)
 {
     GlfsPreopened *entry = NULL;
 
@@ -217,7 +217,7 @@ static void glfs_set_preopened(const char *volume, glfs_t *fs)
     QLIST_INSERT_HEAD(&glfs_list, entry, list);
 }
 
-static glfs_t *glfs_find_preopened(const char *volume)
+static glfs_t *glfs_get_preopened(const char *volume)
 {
     GlfsPreopened *entry = NULL;
 
@@ -231,7 +231,7 @@ static glfs_t *glfs_find_preopened(const char *volume)
     return NULL;
 }
 
-static void glfs_clear_preopened(glfs_t *fs)
+static void glfs_put_preopened(glfs_t *fs)
 {
     GlfsPreopened *entry = NULL;
     GlfsPreopened *next;
@@ -393,7 +393,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
     GlusterServerList *server;
     unsigned long long port;
 
-    glfs = glfs_find_preopened(gconf->volume);
+    glfs = glfs_get_preopened(gconf->volume);
     if (glfs) {
         return glfs;
     }
@@ -403,7 +403,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
         goto out;
     }
 
-    glfs_set_preopened(gconf->volume, glfs);
+    glfs_add_preopened(gconf->volume, glfs);
 
     for (server = gconf->server; server; server = server->next) {
         if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
@@ -463,7 +463,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 out:
     if (glfs) {
         old_errno = errno;
-        glfs_clear_preopened(glfs);
+        glfs_put_preopened(glfs);
         errno = old_errno;
     }
     return NULL;
@@ -844,7 +844,7 @@ out:
         glfs_close(s->fd);
     }
 
-    glfs_clear_preopened(s->glfs);
+    glfs_put_preopened(s->glfs);
 
     return ret;
 }
@@ -913,7 +913,7 @@ static void qemu_gluster_reopen_commit(BDRVReopenState *state)
         glfs_close(s->fd);
     }
 
-    glfs_clear_preopened(s->glfs);
+    glfs_put_preopened(s->glfs);
 
     /* use the newly opened image / connection */
     s->fd         = reop_s->fd;
@@ -938,7 +938,7 @@ static void qemu_gluster_reopen_abort(BDRVReopenState *state)
         glfs_close(reop_s->fd);
     }
 
-    glfs_clear_preopened(reop_s->glfs);
+    glfs_put_preopened(reop_s->glfs);
 
     g_free(state->opaque);
     state->opaque = NULL;
@@ -1062,7 +1062,7 @@ static int qemu_gluster_create(const char *filename,
 out:
     g_free(tmp);
     qapi_free_BlockdevOptionsGluster(gconf);
-    glfs_clear_preopened(glfs);
+    glfs_put_preopened(glfs);
     return ret;
 }
 
@@ -1135,7 +1135,7 @@ static void qemu_gluster_close(BlockDriverState *bs)
         glfs_close(s->fd);
         s->fd = NULL;
     }
-    glfs_clear_preopened(s->glfs);
+    glfs_put_preopened(s->glfs);
 }
 
 static coroutine_fn int qemu_gluster_co_flush_to_disk(BlockDriverState *bs)
-- 
2.9.3

[Qemu-devel] [PATCH 4/4] block/gluster: add missing QLIST_HEAD_INITIALIZER()

[Stefan Hajnoczi]

The "qemu/queue.h" data structures provide static initializer macros.
The QLIST version just initializes to NULL so code happens to work when
the initializer is forgotten.  Other types like SLIST are not so
forgiving because they set fields to non-NULL values.

The initializer macro should always be used for consistency and so that
no errors are introduced when switching between list/queue variants.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/gluster.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/gluster.c b/block/gluster.c
index 181b345..3ac9105 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -63,7 +63,8 @@ typedef struct GlfsPreopened {
     QLIST_ENTRY(GlfsPreopened) list;
 } GlfsPreopened;
 
-static QLIST_HEAD(glfs_list, GlfsPreopened) glfs_list;
+static QLIST_HEAD(glfs_list, GlfsPreopened) glfs_list =
+    QLIST_HEAD_INITIALIZER(glfs_list);
 
 static QemuOptsList qemu_gluster_create_opts = {
     .name = "qemu-gluster-create-opts",
-- 
2.9.3

Re: [Qemu-devel] [PATCH 0/4] block/gluster: cleanups for GlfsPreopened

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

On 01/27/2017 04:00 AM, Stefan Hajnoczi wrote:
> Code added in commit 6349c15410361d3fe52c9beee309954d606f8ccd ("block/gluster:
> memory usage: use one glfs instance per volume") does not follow conventions
> and violates QEMU coding style.  Although any single issue in isolation is not
> worth patching, there are several of these and I think it's worth resolving
> them together in one sweep.
> 
> Stefan Hajnoczi (4):
>   block/gluster: fix wrong indent in glfs_find_preopened()
>   block/gluster: drop intermediate ListElement struct
>   block/gluster: use conventional names for GlfsPreopened functions
>   block/gluster: add missing QLIST_HEAD_INITIALIZER()

For the series,
Reviewed-by: Eric Blake <eblake@redhat.com>

> 
>  block/gluster.c | 66 +++++++++++++++++++++++++++------------------------------
>  1 file changed, 31 insertions(+), 35 deletions(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]