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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list