[Spice-commits] 6 commits - server/Makefile.am server/reds-private.h server/reds.c server/stat-file.c server/stat-file.h server/stat.h server/tests

Frediano Ziglio fziglio at kemper.freedesktop.org
Wed Nov 16 16:39:01 UTC 2016


 server/Makefile.am            |    2 
 server/reds-private.h         |   12 --
 server/reds.c                 |  122 +---------------------
 server/stat-file.c            |  223 ++++++++++++++++++++++++++++++++++++++++++
 server/stat-file.h            |   39 +++++++
 server/stat.h                 |    4 
 server/tests/Makefile.am      |    1 
 server/tests/stat-file-test.c |  105 +++++++++++++++++++
 8 files changed, 381 insertions(+), 127 deletions(-)

New commits:
commit 002c7c9eeb65fd260b6fd9478e607895bffb18be
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed Nov 16 15:22:52 2016 +0000

    Make RedStatFile private inside stat-file.c
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/reds-private.h b/server/reds-private.h
index d293ddc..e7e9ad7 100644
--- a/server/reds-private.h
+++ b/server/reds-private.h
@@ -121,7 +121,7 @@ struct RedsState {
     SSL_CTX *ctx;
 
 #ifdef RED_STATISTICS
-    RedStatFile stat_file;
+    RedStatFile *stat_file;
 #endif
     int allow_multiple_clients;
 
diff --git a/server/reds.c b/server/reds.c
index 061a190..9a3f06e 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -352,22 +352,22 @@ static void reds_link_free(RedLinkInfo *link)
 
 StatNodeRef stat_add_node(RedsState *reds, StatNodeRef parent, const char *name, int visible)
 {
-    return stat_file_add_node(&reds->stat_file, parent, name, visible);
+    return stat_file_add_node(reds->stat_file, parent, name, visible);
 }
 
 void stat_remove_node(RedsState *reds, StatNodeRef ref)
 {
-    stat_file_remove_node(&reds->stat_file, ref);
+    stat_file_remove_node(reds->stat_file, ref);
 }
 
 uint64_t *stat_add_counter(RedsState *reds, StatNodeRef parent, const char *name, int visible)
 {
-    return stat_file_add_counter(&reds->stat_file, parent, name, visible);
+    return stat_file_add_counter(reds->stat_file, parent, name, visible);
 }
 
 void stat_remove_counter(RedsState *reds, uint64_t *counter)
 {
-    stat_file_remove_counter(&reds->stat_file, counter);
+    stat_file_remove_counter(reds->stat_file, counter);
 }
 #endif
 
@@ -2827,7 +2827,7 @@ static int reds_init_ssl(RedsState *reds)
 static void reds_cleanup(RedsState *reds)
 {
 #ifdef RED_STATISTICS
-    stat_file_unlink(&reds->stat_file);
+    stat_file_unlink(reds->stat_file);
 #endif
 }
 
@@ -3359,7 +3359,7 @@ static int do_spice_init(RedsState *reds, SpiceCoreInterface *core_interface)
     }
 
 #ifdef RED_STATISTICS
-    stat_file_init(&reds->stat_file, REDS_MAX_STAT_NODES);
+    reds->stat_file = stat_file_new(REDS_MAX_STAT_NODES);
 #endif
 
     if (reds_init_net(reds) < 0) {
diff --git a/server/stat-file.c b/server/stat-file.c
index 5f54869..d1e25ce 100644
--- a/server/stat-file.c
+++ b/server/stat-file.c
@@ -34,25 +34,36 @@
 #define STAT_SHM_SIZE(max_nodes) \
     (sizeof(SpiceStat) + (max_nodes) * sizeof(SpiceStatNode))
 
-void stat_file_init(RedStatFile *stat_file, unsigned int max_nodes)
+struct RedStatFile {
+    char *shm_name;
+    SpiceStat *stat;
+    pthread_mutex_t lock;
+    unsigned int max_nodes;
+};
+
+RedStatFile *stat_file_new(unsigned int max_nodes)
 {
     int fd;
     size_t shm_size = STAT_SHM_SIZE(max_nodes);
+    RedStatFile *stat_file = spice_new0(RedStatFile, 1);
 
     stat_file->max_nodes = max_nodes;
     stat_file->shm_name = g_strdup_printf(SPICE_STAT_SHM_NAME, getpid());
     shm_unlink(stat_file->shm_name);
     if ((fd = shm_open(stat_file->shm_name, O_CREAT | O_RDWR, 0444)) == -1) {
         spice_error("statistics shm_open failed, %s", strerror(errno));
+        goto cleanup;
     }
     if (ftruncate(fd, shm_size) == -1) {
         close(fd);
         spice_error("statistics ftruncate failed, %s", strerror(errno));
+        goto cleanup;
     }
     stat_file->stat = (SpiceStat *)mmap(NULL, shm_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
     close(fd);
     if (stat_file->stat == (SpiceStat *)MAP_FAILED) {
         spice_error("statistics mmap failed, %s", strerror(errno));
+        goto cleanup;
     }
     memset(stat_file->stat, 0, shm_size);
     stat_file->stat->magic = SPICE_STAT_MAGIC;
@@ -60,7 +71,18 @@ void stat_file_init(RedStatFile *stat_file, unsigned int max_nodes)
     stat_file->stat->root_index = INVALID_STAT_REF;
     if (pthread_mutex_init(&stat_file->lock, NULL)) {
         spice_error("mutex init failed");
+        goto cleanup;
     }
+    return stat_file;
+
+cleanup:
+    free(stat_file);
+    return NULL;
+}
+
+const char *stat_file_get_shm_name(RedStatFile *stat_file)
+{
+    return stat_file->shm_name;
 }
 
 void stat_file_unlink(RedStatFile *stat_file)
diff --git a/server/stat-file.h b/server/stat-file.h
index 8ac8efc..903b68b 100644
--- a/server/stat-file.h
+++ b/server/stat-file.h
@@ -24,15 +24,11 @@
 typedef uint32_t StatNodeRef;
 #define INVALID_STAT_REF (~(StatNodeRef)0)
 
-typedef struct {
-    char *shm_name;
-    SpiceStat *stat;
-    pthread_mutex_t lock;
-    unsigned int max_nodes;
-} RedStatFile;
-
-void stat_file_init(RedStatFile *stat_file, unsigned int max_nodes);
+typedef struct RedStatFile RedStatFile;
+
+RedStatFile *stat_file_new(unsigned int max_nodes);
 void stat_file_unlink(RedStatFile *file_stat);
+const char *stat_file_get_shm_name(RedStatFile *stat_file);
 StatNodeRef stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent,
                                const char *name, int visible);
 uint64_t *stat_file_add_counter(RedStatFile *stat_file, StatNodeRef parent,
diff --git a/server/tests/stat-file-test.c b/server/tests/stat-file-test.c
index 8e0c2f6..09b0c7a 100644
--- a/server/tests/stat-file-test.c
+++ b/server/tests/stat-file-test.c
@@ -34,30 +34,31 @@
 
 static void stat_file(void)
 {
-    RedStatFile stat_file;
+    RedStatFile *stat_file;
     StatNodeRef ref, refs[10];
-    uint64_t *counter, *counters[10];
+    uint64_t *counter, *counters[10] SPICE_GNUC_UNUSED;
     int i;
     char *filename = NULL;
     char name[20];
 
     /* create */
-    stat_file_init(&stat_file, 10);
+    stat_file = stat_file_new(10);
 
-    g_assert_nonnull(stat_file.shm_name);
-    filename = strdup(stat_file.shm_name);
-    g_assert(access(stat_file.shm_name, R_OK));
+    g_assert_nonnull(stat_file);
+    g_assert_nonnull(stat_file_get_shm_name(stat_file));
+    filename = strdup(stat_file_get_shm_name(stat_file));
+    g_assert(access(filename, R_OK));
 
     /* fill all nodes */
     for (i = 0; i < 10; ++i) {
         sprintf(name, "node %d", i);
-        ref = stat_file_add_node(&stat_file, INVALID_STAT_REF, name, TRUE);
+        ref = stat_file_add_node(stat_file, INVALID_STAT_REF, name, TRUE);
         refs[i] = ref;
         g_assert_cmpuint(ref,!=,INVALID_STAT_REF);
     }
 
     /* should fail */
-    ref = stat_file_add_node(&stat_file, INVALID_STAT_REF, "invalid", TRUE);
+    ref = stat_file_add_node(stat_file, INVALID_STAT_REF, "invalid", TRUE);
     g_assert_cmpuint(ref,==,INVALID_STAT_REF);
 
     /* we should find already present nodes */
@@ -66,7 +67,7 @@ static void stat_file(void)
          * As 17 and 10 are coprime numbers you'll get the same numbers
          * after 10 iterations */
         sprintf(name, "node %d", (i * 17 + 5) % 10);
-        ref = stat_file_add_node(&stat_file, INVALID_STAT_REF, name, TRUE);
+        ref = stat_file_add_node(stat_file, INVALID_STAT_REF, name, TRUE);
         g_assert_cmpuint(ref,!=,INVALID_STAT_REF);
     }
 
@@ -74,22 +75,22 @@ static void stat_file(void)
     for (i = 0; i < 6; ++i) {
         /* see above why the formula is used */
         int n = (i * 23 + 3) % 10;
-        stat_file_remove_node(&stat_file, refs[n]);
+        stat_file_remove_node(stat_file, refs[n]);
         refs[n] = INVALID_STAT_REF;
     }
 
     /* now there should be some place for some counters */
     for (i = 0; i < 6; ++i) {
         sprintf(name, "counter %d", i);
-        counter = stat_file_add_counter(&stat_file, INVALID_STAT_REF, name, TRUE);
+        counter = stat_file_add_counter(stat_file, INVALID_STAT_REF, name, TRUE);
         counters[i] = counter;
         g_assert_nonnull(counter);
     }
-    counter = stat_file_add_counter(&stat_file, INVALID_STAT_REF, "invalid", TRUE);
+    counter = stat_file_add_counter(stat_file, INVALID_STAT_REF, "invalid", TRUE);
     g_assert_null(counter);
 
-    stat_file_unlink(&stat_file);
-    g_assert_null(stat_file.shm_name);
+    stat_file_unlink(stat_file);
+    g_assert_null(stat_file_get_shm_name(stat_file));
     g_assert_cmpint(access(filename, F_OK),==,-1);
     free(filename);
 }
commit 41c042ca36f6d2a3fa6a0354befe59a784c1ca44
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed Nov 16 14:51:52 2016 +0000

    Avoid leaking file descriptor for statistics
    
    mmap memory area will remain even if the descriptor is closed.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/stat-file.c b/server/stat-file.c
index e9b4208..5f54869 100644
--- a/server/stat-file.c
+++ b/server/stat-file.c
@@ -46,9 +46,11 @@ void stat_file_init(RedStatFile *stat_file, unsigned int max_nodes)
         spice_error("statistics shm_open failed, %s", strerror(errno));
     }
     if (ftruncate(fd, shm_size) == -1) {
+        close(fd);
         spice_error("statistics ftruncate failed, %s", strerror(errno));
     }
     stat_file->stat = (SpiceStat *)mmap(NULL, shm_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+    close(fd);
     if (stat_file->stat == (SpiceStat *)MAP_FAILED) {
         spice_error("statistics mmap failed, %s", strerror(errno));
     }
commit d163f17a3683fc12b54022668a92580034e7036f
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed Nov 16 14:49:31 2016 +0000

    Use g_strdup_printf instead of manually malloc/snprintf
    
    Make the code easier and shorter.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/stat-file.c b/server/stat-file.c
index 7e35db0..e9b4208 100644
--- a/server/stat-file.c
+++ b/server/stat-file.c
@@ -36,14 +36,11 @@
 
 void stat_file_init(RedStatFile *stat_file, unsigned int max_nodes)
 {
-    int shm_name_len;
     int fd;
     size_t shm_size = STAT_SHM_SIZE(max_nodes);
 
     stat_file->max_nodes = max_nodes;
-    shm_name_len = strlen(SPICE_STAT_SHM_NAME) + 20;
-    stat_file->shm_name = (char *)spice_malloc(shm_name_len);
-    snprintf(stat_file->shm_name, shm_name_len, SPICE_STAT_SHM_NAME, getpid());
+    stat_file->shm_name = g_strdup_printf(SPICE_STAT_SHM_NAME, getpid());
     shm_unlink(stat_file->shm_name);
     if ((fd = shm_open(stat_file->shm_name, O_CREAT | O_RDWR, 0444)) == -1) {
         spice_error("statistics shm_open failed, %s", strerror(errno));
@@ -68,7 +65,7 @@ void stat_file_unlink(RedStatFile *stat_file)
 {
     if (stat_file->shm_name) {
         shm_unlink(stat_file->shm_name);
-        free(stat_file->shm_name);
+        g_free(stat_file->shm_name);
         stat_file->shm_name = NULL;
     }
 }
commit 5209d33977c51f009d9114ba622ec0f30da3dc35
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed Nov 16 12:02:46 2016 +0000

    Add a base test for statistic file
    
    Create a file and add/remove some nodes.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index 8580a9a..8351253 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -43,6 +43,7 @@ check_PROGRAMS =				\
 	test-agent-msg-filter			\
 	test-loop				\
 	test-qxl-parsing			\
+	stat-file-test				\
 	$(NULL)
 
 noinst_PROGRAMS =				\
diff --git a/server/tests/stat-file-test.c b/server/tests/stat-file-test.c
new file mode 100644
index 0000000..8e0c2f6
--- /dev/null
+++ b/server/tests/stat-file-test.c
@@ -0,0 +1,104 @@
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2016 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+#include <config.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <glib.h>
+#include <spice.h>
+
+#include "stat-file.h"
+
+/* GLIB_CHECK_VERSION(2, 40, 0) */
+#ifndef g_assert_nonnull
+#define g_assert_nonnull g_assert
+#endif
+#ifndef g_assert_null
+#define g_assert_null(ptr) g_assert((ptr) == NULL)
+#endif
+
+static void stat_file(void)
+{
+    RedStatFile stat_file;
+    StatNodeRef ref, refs[10];
+    uint64_t *counter, *counters[10];
+    int i;
+    char *filename = NULL;
+    char name[20];
+
+    /* create */
+    stat_file_init(&stat_file, 10);
+
+    g_assert_nonnull(stat_file.shm_name);
+    filename = strdup(stat_file.shm_name);
+    g_assert(access(stat_file.shm_name, R_OK));
+
+    /* fill all nodes */
+    for (i = 0; i < 10; ++i) {
+        sprintf(name, "node %d", i);
+        ref = stat_file_add_node(&stat_file, INVALID_STAT_REF, name, TRUE);
+        refs[i] = ref;
+        g_assert_cmpuint(ref,!=,INVALID_STAT_REF);
+    }
+
+    /* should fail */
+    ref = stat_file_add_node(&stat_file, INVALID_STAT_REF, "invalid", TRUE);
+    g_assert_cmpuint(ref,==,INVALID_STAT_REF);
+
+    /* we should find already present nodes */
+    for (i = 0; i < 10; ++i) {
+        /* the formula used here is to take nodes here and there.
+         * As 17 and 10 are coprime numbers you'll get the same numbers
+         * after 10 iterations */
+        sprintf(name, "node %d", (i * 17 + 5) % 10);
+        ref = stat_file_add_node(&stat_file, INVALID_STAT_REF, name, TRUE);
+        g_assert_cmpuint(ref,!=,INVALID_STAT_REF);
+    }
+
+    /* delete some nodes */
+    for (i = 0; i < 6; ++i) {
+        /* see above why the formula is used */
+        int n = (i * 23 + 3) % 10;
+        stat_file_remove_node(&stat_file, refs[n]);
+        refs[n] = INVALID_STAT_REF;
+    }
+
+    /* now there should be some place for some counters */
+    for (i = 0; i < 6; ++i) {
+        sprintf(name, "counter %d", i);
+        counter = stat_file_add_counter(&stat_file, INVALID_STAT_REF, name, TRUE);
+        counters[i] = counter;
+        g_assert_nonnull(counter);
+    }
+    counter = stat_file_add_counter(&stat_file, INVALID_STAT_REF, "invalid", TRUE);
+    g_assert_null(counter);
+
+    stat_file_unlink(&stat_file);
+    g_assert_null(stat_file.shm_name);
+    g_assert_cmpint(access(filename, F_OK),==,-1);
+    free(filename);
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/server/stat-file", stat_file);
+
+    return g_test_run();
+}
commit fe543c6b459f3b7988183b97a0739e1acdaa8e9f
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Nov 15 23:38:57 2016 +0000

    Fix node removal
    
    Avoid to produce loop in the tree removing and adding nodes.
    Unlink the node from the containing tree.
    This possibly will create unlinked trees if a parent node is
    deleted.
    
    What was happening is that the creation of loops inside
    the tree caused some statistical function to go into an
    infinite loop (and reds_stat tool too).
    
    Nodes were only invalidated but still linked so when reused
    the new node could point to an already existing node (like a
    sibling) which pointed to the new reused one.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/stat-file.c b/server/stat-file.c
index d688f80..7e35db0 100644
--- a/server/stat-file.c
+++ b/server/stat-file.c
@@ -159,10 +159,35 @@ stat_file_add_counter(RedStatFile *stat_file, StatNodeRef parent, const char *na
 
 static void stat_file_remove(RedStatFile *stat_file, SpiceStatNode *node)
 {
+    const StatNodeRef node_ref = node - stat_file->stat->nodes;
+    const StatNodeRef node_next = node->next_sibling_index;
+    StatNodeRef ref;
+
     pthread_mutex_lock(&stat_file->lock);
     node->flags &= ~SPICE_STAT_NODE_FLAG_ENABLED;
     stat_file->stat->generation++;
     stat_file->stat->num_of_nodes--;
+    /* remove links from parent or siblings */
+    /* children will be orphans */
+    if (stat_file->stat->root_index == node_ref) {
+        stat_file->stat->root_index = node_next;
+    } else for (ref = 0; ref <= stat_file->max_nodes; ref++) {
+        node = &stat_file->stat->nodes[ref];
+        if (!(node->flags & SPICE_STAT_NODE_FLAG_ENABLED)) {
+            continue;
+        }
+        /* in a tree there is only a link from a parent or
+         * previous sibling so we can exit the loop as
+         * soon as we found on link */
+        if (node->first_child_index == node_ref) {
+            node->first_child_index = node_next;
+            break;
+        }
+        if (node->next_sibling_index == node_ref) {
+            node->next_sibling_index = node_next;
+            break;
+        }
+    }
     pthread_mutex_unlock(&stat_file->lock);
 }
 
commit f9ccabfaea24b93847f3f269edb8993377a1bc61
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Nov 15 13:51:07 2016 +0000

    Separate code to manage statistic file
    
    Code is quite independent so move in separate file.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/Makefile.am b/server/Makefile.am
index 972f3e2..8d4eae9 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -143,6 +143,8 @@ libserver_la_SOURCES =				\
 	sound.c				\
 	sound.h				\
 	stat.h					\
+	stat-file.c				\
+	stat-file.h				\
 	spicevmc.c				\
 	video-encoder.h				\
 	zlib-encoder.c				\
diff --git a/server/reds-private.h b/server/reds-private.h
index 8198a8d..d293ddc 100644
--- a/server/reds-private.h
+++ b/server/reds-private.h
@@ -24,6 +24,7 @@
 #include "main-dispatcher.h"
 #include "main-channel.h"
 #include "inputs-channel.h"
+#include "stat-file.h"
 
 #define MIGRATE_TIMEOUT (MSEC_PER_SEC * 10)
 #define MM_TIME_DELTA 400 /*ms*/
@@ -45,13 +46,6 @@ typedef struct MonitorMode {
     uint32_t y_res;
 } MonitorMode;
 
-#ifdef RED_STATISTICS
-
-#define REDS_MAX_STAT_NODES 100
-#define REDS_STAT_SHM_SIZE (sizeof(SpiceStat) + REDS_MAX_STAT_NODES * sizeof(SpiceStatNode))
-
-#endif
-
 typedef struct RedsMigPendingLink {
     SpiceLinkMess *link_msg;
     RedsStream *stream;
@@ -127,9 +121,7 @@ struct RedsState {
     SSL_CTX *ctx;
 
 #ifdef RED_STATISTICS
-    char *stat_shm_name;
-    SpiceStat *stat;
-    pthread_mutex_t stat_lock;
+    RedStatFile stat_file;
 #endif
     int allow_multiple_clients;
 
diff --git a/server/reds.c b/server/reds.c
index 12a274c..061a190 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -76,6 +76,8 @@
 #include "main-channel-client.h"
 #include "red-client.h"
 
+#define REDS_MAX_STAT_NODES 100
+
 static void reds_client_monitors_config(RedsState *reds, VDAgentMonitorsConfig *monitors_config);
 static gboolean reds_use_client_monitors_config(RedsState *reds);
 
@@ -348,105 +350,24 @@ static void reds_link_free(RedLinkInfo *link)
 
 #ifdef RED_STATISTICS
 
-static void reds_insert_stat_node(RedsState *reds, StatNodeRef parent, StatNodeRef ref)
-{
-    SpiceStatNode *node = &reds->stat->nodes[ref];
-    uint32_t pos = INVALID_STAT_REF;
-    uint32_t node_index;
-    uint32_t *head;
-    SpiceStatNode *n;
-
-    node->first_child_index = INVALID_STAT_REF;
-    head = (parent == INVALID_STAT_REF ? &reds->stat->root_index :
-                                         &reds->stat->nodes[parent].first_child_index);
-    node_index = *head;
-    while (node_index != INVALID_STAT_REF && (n = &reds->stat->nodes[node_index]) &&
-                                                     strcmp(node->name, n->name) > 0) {
-        pos = node_index;
-        node_index = n->next_sibling_index;
-    }
-    if (pos == INVALID_STAT_REF) {
-        node->next_sibling_index = *head;
-        *head = ref;
-    } else {
-        n = &reds->stat->nodes[pos];
-        node->next_sibling_index = n->next_sibling_index;
-        n->next_sibling_index = ref;
-    }
-}
-
 StatNodeRef stat_add_node(RedsState *reds, StatNodeRef parent, const char *name, int visible)
 {
-    StatNodeRef ref;
-    SpiceStatNode *node;
-
-    spice_assert(name && strlen(name) > 0);
-    if (strlen(name) >= sizeof(node->name)) {
-        return INVALID_STAT_REF;
-    }
-    pthread_mutex_lock(&reds->stat_lock);
-    ref = (parent == INVALID_STAT_REF ? reds->stat->root_index :
-                                        reds->stat->nodes[parent].first_child_index);
-    while (ref != INVALID_STAT_REF) {
-        node = &reds->stat->nodes[ref];
-        if (strcmp(name, node->name)) {
-            ref = node->next_sibling_index;
-        } else {
-            pthread_mutex_unlock(&reds->stat_lock);
-            return ref;
-        }
-    }
-    if (reds->stat->num_of_nodes >= REDS_MAX_STAT_NODES || reds->stat == NULL) {
-        pthread_mutex_unlock(&reds->stat_lock);
-        return INVALID_STAT_REF;
-    }
-    reds->stat->generation++;
-    reds->stat->num_of_nodes++;
-    for (ref = 0; ref <= REDS_MAX_STAT_NODES; ref++) {
-        node = &reds->stat->nodes[ref];
-        if (!(node->flags & SPICE_STAT_NODE_FLAG_ENABLED)) {
-            break;
-        }
-    }
-    spice_assert(!(node->flags & SPICE_STAT_NODE_FLAG_ENABLED));
-    node->value = 0;
-    node->flags = SPICE_STAT_NODE_FLAG_ENABLED | (visible ? SPICE_STAT_NODE_FLAG_VISIBLE : 0);
-    g_strlcpy(node->name, name, sizeof(node->name));
-    reds_insert_stat_node(reds, parent, ref);
-    pthread_mutex_unlock(&reds->stat_lock);
-    return ref;
-}
-
-static void reds_stat_remove(RedsState *reds, SpiceStatNode *node)
-{
-    pthread_mutex_lock(&reds->stat_lock);
-    node->flags &= ~SPICE_STAT_NODE_FLAG_ENABLED;
-    reds->stat->generation++;
-    reds->stat->num_of_nodes--;
-    pthread_mutex_unlock(&reds->stat_lock);
+    return stat_file_add_node(&reds->stat_file, parent, name, visible);
 }
 
 void stat_remove_node(RedsState *reds, StatNodeRef ref)
 {
-    reds_stat_remove(reds, &reds->stat->nodes[ref]);
+    stat_file_remove_node(&reds->stat_file, ref);
 }
 
 uint64_t *stat_add_counter(RedsState *reds, StatNodeRef parent, const char *name, int visible)
 {
-    StatNodeRef ref = stat_add_node(reds, parent, name, visible);
-    SpiceStatNode *node;
-
-    if (ref == INVALID_STAT_REF) {
-        return NULL;
-    }
-    node = &reds->stat->nodes[ref];
-    node->flags |= SPICE_STAT_NODE_FLAG_VALUE;
-    return &node->value;
+    return stat_file_add_counter(&reds->stat_file, parent, name, visible);
 }
 
 void stat_remove_counter(RedsState *reds, uint64_t *counter)
 {
-    reds_stat_remove(reds, (SpiceStatNode *)(counter - SPICE_OFFSETOF(SpiceStatNode, value)));
+    stat_file_remove_counter(&reds->stat_file, counter);
 }
 #endif
 
@@ -2906,11 +2827,7 @@ static int reds_init_ssl(RedsState *reds)
 static void reds_cleanup(RedsState *reds)
 {
 #ifdef RED_STATISTICS
-    if (reds->stat_shm_name) {
-        shm_unlink(reds->stat_shm_name);
-        free(reds->stat_shm_name);
-        reds->stat_shm_name = NULL;
-    }
+    stat_file_unlink(&reds->stat_file);
 #endif
 }
 
@@ -3442,30 +3359,7 @@ static int do_spice_init(RedsState *reds, SpiceCoreInterface *core_interface)
     }
 
 #ifdef RED_STATISTICS
-    int shm_name_len;
-    int fd;
-
-    shm_name_len = strlen(SPICE_STAT_SHM_NAME) + 20;
-    reds->stat_shm_name = (char *)spice_malloc(shm_name_len);
-    snprintf(reds->stat_shm_name, shm_name_len, SPICE_STAT_SHM_NAME, getpid());
-    shm_unlink(reds->stat_shm_name);
-    if ((fd = shm_open(reds->stat_shm_name, O_CREAT | O_RDWR, 0444)) == -1) {
-        spice_error("statistics shm_open failed, %s", strerror(errno));
-    }
-    if (ftruncate(fd, REDS_STAT_SHM_SIZE) == -1) {
-        spice_error("statistics ftruncate failed, %s", strerror(errno));
-    }
-    reds->stat = (SpiceStat *)mmap(NULL, REDS_STAT_SHM_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
-    if (reds->stat == (SpiceStat *)MAP_FAILED) {
-        spice_error("statistics mmap failed, %s", strerror(errno));
-    }
-    memset(reds->stat, 0, REDS_STAT_SHM_SIZE);
-    reds->stat->magic = SPICE_STAT_MAGIC;
-    reds->stat->version = SPICE_STAT_VERSION;
-    reds->stat->root_index = INVALID_STAT_REF;
-    if (pthread_mutex_init(&reds->stat_lock, NULL)) {
-        spice_error("mutex init failed");
-    }
+    stat_file_init(&reds->stat_file, REDS_MAX_STAT_NODES);
 #endif
 
     if (reds_init_net(reds) < 0) {
diff --git a/server/stat-file.c b/server/stat-file.c
new file mode 100644
index 0000000..d688f80
--- /dev/null
+++ b/server/stat-file.c
@@ -0,0 +1,177 @@
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2009-2016 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <common/log.h>
+#include <common/mem.h>
+
+#include "stat-file.h"
+
+#define STAT_SHM_SIZE(max_nodes) \
+    (sizeof(SpiceStat) + (max_nodes) * sizeof(SpiceStatNode))
+
+void stat_file_init(RedStatFile *stat_file, unsigned int max_nodes)
+{
+    int shm_name_len;
+    int fd;
+    size_t shm_size = STAT_SHM_SIZE(max_nodes);
+
+    stat_file->max_nodes = max_nodes;
+    shm_name_len = strlen(SPICE_STAT_SHM_NAME) + 20;
+    stat_file->shm_name = (char *)spice_malloc(shm_name_len);
+    snprintf(stat_file->shm_name, shm_name_len, SPICE_STAT_SHM_NAME, getpid());
+    shm_unlink(stat_file->shm_name);
+    if ((fd = shm_open(stat_file->shm_name, O_CREAT | O_RDWR, 0444)) == -1) {
+        spice_error("statistics shm_open failed, %s", strerror(errno));
+    }
+    if (ftruncate(fd, shm_size) == -1) {
+        spice_error("statistics ftruncate failed, %s", strerror(errno));
+    }
+    stat_file->stat = (SpiceStat *)mmap(NULL, shm_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+    if (stat_file->stat == (SpiceStat *)MAP_FAILED) {
+        spice_error("statistics mmap failed, %s", strerror(errno));
+    }
+    memset(stat_file->stat, 0, shm_size);
+    stat_file->stat->magic = SPICE_STAT_MAGIC;
+    stat_file->stat->version = SPICE_STAT_VERSION;
+    stat_file->stat->root_index = INVALID_STAT_REF;
+    if (pthread_mutex_init(&stat_file->lock, NULL)) {
+        spice_error("mutex init failed");
+    }
+}
+
+void stat_file_unlink(RedStatFile *stat_file)
+{
+    if (stat_file->shm_name) {
+        shm_unlink(stat_file->shm_name);
+        free(stat_file->shm_name);
+        stat_file->shm_name = NULL;
+    }
+}
+
+static void reds_insert_stat_node(RedStatFile *stat_file, StatNodeRef parent, StatNodeRef ref)
+{
+    SpiceStatNode *node = &stat_file->stat->nodes[ref];
+    uint32_t pos = INVALID_STAT_REF;
+    uint32_t node_index;
+    uint32_t *head;
+    SpiceStatNode *n;
+
+    node->first_child_index = INVALID_STAT_REF;
+    head = (parent == INVALID_STAT_REF ? &stat_file->stat->root_index :
+                                         &stat_file->stat->nodes[parent].first_child_index);
+    node_index = *head;
+    while (node_index != INVALID_STAT_REF && (n = &stat_file->stat->nodes[node_index]) &&
+                                                     strcmp(node->name, n->name) > 0) {
+        pos = node_index;
+        node_index = n->next_sibling_index;
+    }
+    if (pos == INVALID_STAT_REF) {
+        node->next_sibling_index = *head;
+        *head = ref;
+    } else {
+        n = &stat_file->stat->nodes[pos];
+        node->next_sibling_index = n->next_sibling_index;
+        n->next_sibling_index = ref;
+    }
+}
+
+StatNodeRef
+stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char *name, int visible)
+{
+    StatNodeRef ref;
+    SpiceStatNode *node;
+
+    spice_assert(name && strlen(name) > 0);
+    if (strlen(name) >= sizeof(node->name)) {
+        return INVALID_STAT_REF;
+    }
+    pthread_mutex_lock(&stat_file->lock);
+    ref = (parent == INVALID_STAT_REF ? stat_file->stat->root_index :
+                                        stat_file->stat->nodes[parent].first_child_index);
+    while (ref != INVALID_STAT_REF) {
+        node = &stat_file->stat->nodes[ref];
+        if (strcmp(name, node->name)) {
+            ref = node->next_sibling_index;
+        } else {
+            pthread_mutex_unlock(&stat_file->lock);
+            return ref;
+        }
+    }
+    if (stat_file->stat->num_of_nodes >= stat_file->max_nodes || stat_file->stat == NULL) {
+        pthread_mutex_unlock(&stat_file->lock);
+        return INVALID_STAT_REF;
+    }
+    stat_file->stat->generation++;
+    stat_file->stat->num_of_nodes++;
+    for (ref = 0; ref <= stat_file->max_nodes; ref++) {
+        node = &stat_file->stat->nodes[ref];
+        if (!(node->flags & SPICE_STAT_NODE_FLAG_ENABLED)) {
+            break;
+        }
+    }
+    spice_assert(!(node->flags & SPICE_STAT_NODE_FLAG_ENABLED));
+    node->value = 0;
+    node->flags = SPICE_STAT_NODE_FLAG_ENABLED | (visible ? SPICE_STAT_NODE_FLAG_VISIBLE : 0);
+    g_strlcpy(node->name, name, sizeof(node->name));
+    reds_insert_stat_node(stat_file, parent, ref);
+    pthread_mutex_unlock(&stat_file->lock);
+    return ref;
+}
+
+uint64_t *
+stat_file_add_counter(RedStatFile *stat_file, StatNodeRef parent, const char *name, int visible)
+{
+    StatNodeRef ref = stat_file_add_node(stat_file, parent, name, visible);
+    SpiceStatNode *node;
+
+    if (ref == INVALID_STAT_REF) {
+        return NULL;
+    }
+    node = &stat_file->stat->nodes[ref];
+    node->flags |= SPICE_STAT_NODE_FLAG_VALUE;
+    return &node->value;
+}
+
+static void stat_file_remove(RedStatFile *stat_file, SpiceStatNode *node)
+{
+    pthread_mutex_lock(&stat_file->lock);
+    node->flags &= ~SPICE_STAT_NODE_FLAG_ENABLED;
+    stat_file->stat->generation++;
+    stat_file->stat->num_of_nodes--;
+    pthread_mutex_unlock(&stat_file->lock);
+}
+
+void stat_file_remove_node(RedStatFile *stat_file, StatNodeRef ref)
+{
+    stat_file_remove(stat_file, &stat_file->stat->nodes[ref]);
+}
+
+void stat_file_remove_counter(RedStatFile *stat_file, uint64_t *counter)
+{
+    stat_file_remove(stat_file, (SpiceStatNode *)(counter - SPICE_OFFSETOF(SpiceStatNode, value)));
+}
diff --git a/server/stat-file.h b/server/stat-file.h
new file mode 100644
index 0000000..8ac8efc
--- /dev/null
+++ b/server/stat-file.h
@@ -0,0 +1,43 @@
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2016 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+#ifndef STAT_FILE_H_
+#define STAT_FILE_H_
+
+#include <pthread.h>
+#include <spice/stats.h>
+
+typedef uint32_t StatNodeRef;
+#define INVALID_STAT_REF (~(StatNodeRef)0)
+
+typedef struct {
+    char *shm_name;
+    SpiceStat *stat;
+    pthread_mutex_t lock;
+    unsigned int max_nodes;
+} RedStatFile;
+
+void stat_file_init(RedStatFile *stat_file, unsigned int max_nodes);
+void stat_file_unlink(RedStatFile *file_stat);
+StatNodeRef stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent,
+                               const char *name, int visible);
+uint64_t *stat_file_add_counter(RedStatFile *stat_file, StatNodeRef parent,
+                                const char *name, int visible);
+void stat_file_remove_node(RedStatFile *stat_file, StatNodeRef ref);
+void stat_file_remove_counter(RedStatFile *stat_file, uint64_t *counter);
+
+#endif /* STAT_FILE_H_ */
diff --git a/server/stat.h b/server/stat.h
index 7bf90da..a5df7ea 100644
--- a/server/stat.h
+++ b/server/stat.h
@@ -22,9 +22,7 @@
 #include <glib.h>
 
 #include "spice.h"
-
-typedef uint32_t StatNodeRef;
-#define INVALID_STAT_REF (~(StatNodeRef)0)
+#include "stat-file.h"
 
 #ifdef RED_STATISTICS
 StatNodeRef stat_add_node(SpiceServer *reds, StatNodeRef parent, const char *name, int visible);


More information about the Spice-commits mailing list