Re: [libvirt] [PATCH 2/6] conf: add storage_event handling and unittests

9 Jun 2016

Subject mentions unittests but there's no unittests here, they are in patch #3

On 06/09/2016 02:25 PM, Jovanka Gulicoska wrote:
...
Following patern of network_event.c, the following functions are added:
The internal API bits added aren't really interesting IMO. How about just

Add storage event handling infrastructure to storage_event.[ch], following
the network_event.[ch] pattern.
...
virStoragePoolEventLifecycleNew:
create a new storage pool lifecycle event
virStoragePoolEventStateRegisterClient:
Returns number of registerd callbacks
virStoragePoolEventStateRegisterID
---
 src/Makefile.am          |   5 +
 src/conf/object_event.c  |   1 +
 src/conf/storage_conf.h  |   5 +
 src/conf/storage_event.c | 237 +++++++++++++++++++++++++++++++++++++++++++++++
 src/conf/storage_event.h |  60 ++++++++++++
 src/libvirt_private.syms |   5 +
 6 files changed, 313 insertions(+)
 create mode 100644 src/conf/storage_event.c
 create mode 100644 src/conf/storage_event.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 8c83b0c..f0ad5eb 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -340,6 +340,9 @@ DOMAIN_EVENT_SOURCES =						\
 NETWORK_EVENT_SOURCES =						\
      conf/network_event.c conf/network_event.h
+STORAGE_POOL_EVENT_SOURCES =                                    \
+                conf/storage_event.c conf/storage_event.h
+
 # Network driver generic impl APIs
 NETWORK_CONF_SOURCES =						\
      conf/network_conf.c conf/network_conf.h \
@@ -390,6 +393,7 @@ CONF_SOURCES =							\
      $(OBJECT_EVENT_SOURCES)				\
      $(DOMAIN_EVENT_SOURCES)				\
      $(NETWORK_EVENT_SOURCES)			\
+                $(STORAGE_POOL_EVENT_SOURCES)                   \
      $(NETWORK_CONF_SOURCES)				\
      $(NWFILTER_CONF_SOURCES)			\
      $(NODE_DEVICE_CONF_SOURCES)			\
@@ -2352,6 +2356,7 @@ libvirt_setuid_rpc_client_la_SOURCES = 		\
      conf/domain_event.c		\
      conf/network_event.c		\
      conf/object_event.c		\
+                conf/storage_event.c            \
      rpc/virnetsocket.c		\
      rpc/virnetsocket.h		\
      rpc/virnetmessage.h		\
This file uses hard tabs, but you seem to be using spaces.
...
diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index 06eedff..7e6fc79 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -26,6 +26,7 @@
#include "domain_event.h"
 #include "network_event.h"
+#include "storage_event.h"
 #include "object_event.h"
 #include "object_event_private.h"
 #include "virlog.h"
Hmm, is this actually needed here? I don't see why it would. Maybe the
network_event.h is bogus too, but if so, removing it would be a separate patch
...
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 54116a6..8622ed7 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -31,6 +31,8 @@
 # include "virthread.h"
 # include "device_conf.h"
 # include "node_device_conf.h"
+# include "storage_conf.h"
This is trying to import itself :) It can probably be dropped
...
+# include "object_event.h"
# include <libxml/tree.h>
@@ -296,6 +298,9 @@ struct _virStorageDriverState {
     char *autostartDir;
     char *stateDir;
     bool privileged;
+
+    /* Immutable pointer, self-locking APIs */
+    virObjectEventStatePtr storageEventState;
 };
typedef struct _virStoragePoolSourceList virStoragePoolSourceList;
diff --git a/src/conf/storage_event.c b/src/conf/storage_event.c
new file mode 100644
index 0000000..5233d57
--- /dev/null
+++ b/src/conf/storage_event.c
@@ -0,0 +1,237 @@
+/*
+ * storage_event.c: storage event queue processing helpers
+ *
+ * Copyright (C) 2010-2014 Red Hat, Inc.
+ * Copyright (C) 2008 VirtualIron
+ * Copyright (C) 2013 SUSE LINUX Products GmbH, Nuernberg, Germany.
+ *
Not really sure how the copyright should work here... since it's largely based
on other code and this is the copyright notice from network_event.c ... maybe
just leave it like this
...
+ * 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 "storage_event.h"
+#include "object_event.h"
+#include "object_event_private.h"
+#include "datatypes.h"
+#include "virlog.h"
+
+VIR_LOG_INIT("conf.storage_event");
+
+struct _virStoragePoolEvent {
+    virObjectEvent parent;
+
+    /* Unused attribute to allow for subclass creation */
+    bool dummy;
+};
+typedef struct _virStoragePoolEvent virStoragePoolEvent;
+typedef virStoragePoolEvent *virStoragePoolEventPtr;
+
+struct _virStoragePoolEventLifecycle {
+    virStoragePoolEvent parent;
+
+    int type;
+    int detail;
+};
+typedef struct _virStoragePoolEventLifecycle virStoragePoolEventLifecycle;
+typedef virStoragePoolEventLifecycle *virStoragePoolEventLifecyclePtr;
+
+static virClassPtr virStoragePoolEventClass;
+static virClassPtr virStoragePoolEventLifecycleClass;
+static void virStoragePoolEventDispose(void *obj);
+static void virStoragePoolEventLifecycleDispose(void *obj);
+
+static int
+virStoragePoolEventsOnceInit(void)
+{
+    if (!(virStoragePoolEventClass =
+          virClassNew(virClassForObjectEvent(),
+                      "virStoragePoolEvent",
+                      sizeof(virStoragePoolEvent),
+                      virStoragePoolEventDispose)))
+        return -1;
+    if (!(virStoragePoolEventLifecycleClass =
+          virClassNew(virStoragePoolEventClass,
+                      "virStoragePoolEventLifecycle",
+                      sizeof(virStoragePoolEventLifecycle),
+                      virStoragePoolEventLifecycleDispose)))
+        return -1;
+    return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virStoragePoolEvents)
+
+static void
+virStoragePoolEventDispose(void *obj)
+{
+    virStoragePoolEventPtr event = obj;
+    VIR_DEBUG("obj=%p", event);
+}
+
+
+static void
+virStoragePoolEventLifecycleDispose(void *obj)
+{
+    virStoragePoolEventLifecyclePtr event = obj;
+    VIR_DEBUG("obj=%p", event);
+}
+
+
+static void
+virStoragePoolEventDispatchDefaultFunc(virConnectPtr conn,
+                                       virObjectEventPtr event,
+                                       virConnectObjectEventGenericCallback cb,
+                                       void *cbopaque)
+{
+    virStoragePoolPtr pool = virGetStoragePool(conn,
+                                               event->meta.name,
+                                               event->meta.uuid,
+                                               NULL, NULL);
+    if (!pool)
+        return;
+
+    switch ((virStoragePoolEventID)event->eventID) {
+    case VIR_STORAGE_POOL_EVENT_ID_LIFECYCLE:
+        {
+            virStoragePoolEventLifecyclePtr storagePoolLifecycleEvent;
+
+            storagePoolLifecycleEvent = (virStoragePoolEventLifecyclePtr)event;
+            ((virConnectStoragePoolEventLifecycleCallback)cb)(conn, pool,
+                                                          storagePoolLifecycleEvent->type,
+                                                          storagePoolLifecycleEvent->detail,
+                                                          cbopaque);
Indentation is off with these 3 lines

Otherwise this looks fine to me, this is largely just boilerplate that matches
network_event.c impl

Thanks,
Cole

    

Re: [libvirt] [PATCH 2/6] conf: add storage_event handling and unittests

Cole Robinson