Re: [PATCH 1/6] Introduce storage lifecycle event APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/09/2016 02:25 PM, Jovanka Gulicoska wrote:
> virConnectStoragePoolEventRegisterAny:
> Adds a callback to receive notifications of storage pool events
> occurring on a storage pool. Returns a positive integer identifier for
> the callback
> 
> virConnectStoragePoolEventDeregisterAny:
> Removes an event callback
> 
> virConnectStoragePoolEventGenericCallback:
> A generic storage pool event callback handler, for use with
> virStoragePoolEventRegisterAny().
> 
> VIR_STORAGE_POOL_EVENT_CALLBACK:
> Used to cast the event specific callback into the generic one
> for use for virConnectionStoragePoolEventRegisterAny()
> 
> virStoragePoolEventID:
> An enumeration of supported eventIDs for virStoragePoolEventRegisterAny()
> 
> virStoragePoolEventLifecycleType (VIR_STORAGE_POOL_EVENT_ID_LIFECYCLE):
> emitting during storage pool lifecycle events
> 
> virConnectStoragePoolEventLifecycleCallback:
> This callback occurs when pool is started, stopped or refreshed. Used
> when registering an event of type VIR_STORAGE_POOL_EVENT_ID_LIFECYCLE with
> virStoragePoolEvenTrgisterAny()
> ---
>  include/libvirt/libvirt-storage.h |  94 +++++++++++++++++++++++++++++
>  src/datatypes.h                   |  13 ++++
>  src/driver-storage.h              |  14 +++++
>  src/libvirt-storage.c             | 122 ++++++++++++++++++++++++++++++++++++++
>  src/libvirt_public.syms           |   7 +++
>  5 files changed, 250 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-storage.h b/include/libvirt/libvirt-storage.h
> index db6f2b4..cfbdedf 100644
> --- a/include/libvirt/libvirt-storage.h
> +++ b/include/libvirt/libvirt-storage.h
> @@ -377,5 +377,99 @@ int                     virStorageVolResize             (virStorageVolPtr vol,
>  int virStoragePoolIsActive(virStoragePoolPtr pool);
>  int virStoragePoolIsPersistent(virStoragePoolPtr pool);
>  
> +/**
> + * VIR_STORAGE_POOL_EVENT_CALLBACK:
> + *
> + * Used to cast the event specific callback into the generic one
> + * for use for virConnectStoragePoolEventRegisterAny()
> + */
> +# define VIR_STORAGE_POOL_EVENT_CALLBACK(cb)  ((virConnectStoragePoolEventGenericCallback)(cb))
> +

There's an extra space here ')  ('

> +/**
> + * virStoragePoolEventID:
> + *
> + * An enumeration of supported eventId parameters for
> + * virConnectStoragePoolEventRegisterAny().  Each event id determines which
> + * signature of callback function will be used.
> + */
> +typedef enum {
> +    VIR_STORAGE_POOL_EVENT_ID_LIFECYCLE = 0, /* virConnectStoragePoolEventLifecycleCallback */
> +
> +# ifdef VIR_ENUM_SENTINELS
> +    VIR_STORAGE_POOL_EVENT_ID_LAST
> +    /*
> +     * NB: this enum value will increase over time as new events are
> +     * added to the libvirt API. It reflects the last event ID supported
> +     * by this version of the libvirt API.
> +     */
> +# endif
> +} virStoragePoolEventID;
> +
> +/**
> + * virConnectStoragePoolEventGenericCallback:
> + * @conn: the connection pointer
> + * @pool: the pool pointer
> + * @opaque: application specified data
> + *
> + * A generic storage pool event callback handler, for use with
> + * virConnectStoragePoolEventRegisterAny().
> + * Spcific events usually have a customization with extra paramenters,
> + * often with @opaque being passed in a different parameter possition;

*Specific *parameters *position

> + * use VIR_STORAGE_POOL_EVENT_CALLBACK() when registering an
> + * appropriate handler.
> + */
> +typedef void (*virConnectStoragePoolEventGenericCallback)(virConnectPtr conn,
> +                                                          virStoragePool pool,

This should be virStoragePoolPtr pool

> +                                                          void *opaque);
> +
> +/* Use VIR_STORAGE_POOL_EVENT_CALLBACK() to cast the 'cb' parameter  */
> +int virConnectStoragePoolEventRegisterAny(virConnectPtr conn,
> +                                      virStoragePoolPtr pool, /* optional, to filter */
> +                                      int eventID,
> +                                      virConnectStoragePoolEventGenericCallback cb,
> +                                      void *opaque,
> +                                      virFreeCallback freecb);

Indentation is off here, please align these lines with the open parentheses

> +
> +int virConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
> +                                            int callbackID);
> +
> +/**
> + * virStoragePoolEventLifecycleType:
> + *
> + * a virStoragePoolEventLifecycleType is emitted during storage pool
> + * lifecycle events
> + */
> +typedef enum {
> +    VIR_STORAGE_POOL_EVENT_DEFINED = 0,
> +    VIR_STORAGE_POOL_EVENT_UNDEFINED = 1,
> +    VIR_STORAGE_POOL_EVENT_STARTED = 2,
> +    VIR_STORAGE_POOL_EVENT_STOPPED = 3,
> +    VIR_STORAGE_POOL_EVENT_REFRESHED = 4,
> +
> +# ifdef VIR_ENUM_SENTINELS
> +    VIR_STORAGE_POOL_EVENT_LAST
> +# endif
> +} virStoragePoolEventLifecycleType;
> +
> +/**
> + * virConnectStoragePoolEventLifecycleCallback:
> + * @conn: connection object
> + * @pool: pool on which the event occurred
> + * @event: The specific virStoragePoolEventLifeCycleType which occurred
> + * @detail: contains some details on the reason of the event.
> + *          It will be 0 for the while.

I know this mirrors the network event comment, but maybe just change the
phrasing to

@detail: may contain some details on the reason for the event.

and drop the 'It will be 0...'

> + * @opaque: application specified data
> + *
> + * This callback occurs when the pool is started, stopped or refreshed.
> + *

This isn't wholly accurate. Maybe like:

This callback is called when a pool lifecycle action is performed, like start
or stop.

> + * The callback signature to use when registering for an event of type
> + * VIR_STORAGE_POOL_EVENT_ID_LIFECYCLE with
> + * virConnectStoragePoolEventRegisterAny()
> + */
> +typedef void (*virConnectStoragePoolEventLifecycleCallback)(virConnectPtr conn,
> +                                                            virStoragePoolPtr pool,
> +                                                            int event,
> +                                                            int detail,
> +                                                            void *opaque);
>  
>  #endif /* __VIR_LIBVIRT_STORAGE_H__ */
> diff --git a/src/datatypes.h b/src/datatypes.h
> index 8ccc7b0..638bd23 100644
> --- a/src/datatypes.h
> +++ b/src/datatypes.h
> @@ -143,6 +143,19 @@ extern virClassPtr virAdmClientClass;
>          }                                                               \
>      } while (0)
>  
> +# define virCheckStoragePoolGoto(obj, label)                            \
> +    do {                                                                \
> +        virStoragePoolPtr _pool= (obj);                                \
> +        if (!virObjectIsClass(_pool, virStoragePoolClass) ||            \
> +            !virObjectIsClass(_pool->conn, virConnectClass)) {          \
> +            virReportErrorHelper(VIR_FROM_STORAGE,                      \
> +                                 VIR_ERR_INVALID_STORAGE_POOL,          \
> +                                 __FILE__, __FUNCTION__, __LINE__,      \
> +                                 __FUNCTION__);                         \
> +            goto label;                                                 \
> +        }                                                               \
> +    } while (0)
> +
>  # define virCheckStorageVolReturn(obj, retval)                          \
>      do {                                                                \
>          virStorageVolPtr _vol = (obj);                                  \
> diff --git a/src/driver-storage.h b/src/driver-storage.h
> index 0489647..70a1dcc 100644
> --- a/src/driver-storage.h
> +++ b/src/driver-storage.h
> @@ -196,6 +196,18 @@ typedef int
>  typedef int
>  (*virDrvStoragePoolIsPersistent)(virStoragePoolPtr pool);
>  
> +typedef int
> +(*virDrvConnectStoragePoolEventRegisterAny)(virConnectPtr conn,
> +                                            virStoragePoolPtr pool,
> +                                            int eventID,
> +                                            virConnectStoragePoolEventGenericCallback cb,
> +                                            void *opaque,
> +                                            virFreeCallback freecb);
> +
> +typedef int
> +(*virDrvConnectStoragePoolEventDeregisterAny)(virConnectPtr conn,
> +                                              int callbackID);
> +
>  
>  
>  typedef struct _virStorageDriver virStorageDriver;
> @@ -215,6 +227,8 @@ struct _virStorageDriver {
>      virDrvConnectListDefinedStoragePools connectListDefinedStoragePools;
>      virDrvConnectListAllStoragePools connectListAllStoragePools;
>      virDrvConnectFindStoragePoolSources connectFindStoragePoolSources;
> +    virDrvConnectStoragePoolEventRegisterAny connectStoragePoolEventRegisterAny;
> +    virDrvConnectStoragePoolEventDeregisterAny connectStoragePoolEventDeregisterAny;
>      virDrvStoragePoolLookupByName storagePoolLookupByName;
>      virDrvStoragePoolLookupByUUID storagePoolLookupByUUID;
>      virDrvStoragePoolLookupByVolume storagePoolLookupByVolume;
> diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
> index 1ce6745..33362ea 100644
> --- a/src/libvirt-storage.c
> +++ b/src/libvirt-storage.c
> @@ -2124,3 +2124,125 @@ virStoragePoolIsPersistent(virStoragePoolPtr pool)
>      virDispatchError(pool->conn);
>      return -1;
>  }
> +
> +/**
> + * virConnectStoragePoolEventRegisterAny:
> + * @conn: pointer to the connection
> + * @pool: pointer to the storage pool
> + * @eventID: the event type to receive
> + * @cb: callback to the function handling network events
> + * @opaque: opaque data to pass on to the callback
> + * @freecb: optional function to deallocate opaque when not used anymore
> + *
> + * Adds a callback to receive notifications of arbitrary storage pool events
> + * occurring on a storage pool. This function requires that an event loop
> + * has been previously registered with virEventRegisterImpl() or
> + * virEventRegisterDefaultImpl().
> + *
> + * If @pool is NULL, then events will be monitored for any storage pool.
> + * If @pool is non-NULL, then only the specific storage pool will be monitored.
> + *
> + * Most types of event have a callback providing a custom set of parameters

types of events

> + * for the event. When registering an event, it is thus necessary to use
> + * the VIR_STORAGE_POOL_EVENT_CALLBACK() macro to cast the supplied function pointer

this line is too long, move 'pointer' to the next line

> + * to match the signature of this method.
> + *
> + * The virStoragePoolPtr object handle passed into the callback upon delivery
> + * of an event is only valid for the duration of execution of the callback.
> + * If the callback wishes to keep the storage pool object after the callback
> + * returns, it shall take a reference to it, by calling virStoragePoolRef().
> + * The reference can be released once the object is no longer required
> + * by calling virStoragePoolFree().
> + *
> + * The return value from this method is a positive integer identifier
> + * for the callback. To unregister a callback, this callback ID should
> + * be passed to the virConnectStoragePoolEventDeregisterAny() method.
> + *
> + * Returns a callback identifier on success, -1 on failure.
> + */
> +int
> +virConnectStoragePoolEventRegisterAny(virConnectPtr conn,
> +                                      virStoragePoolPtr pool,
> +                                      int eventID,
> +                                      virConnectStoragePoolEventGenericCallback cb,
> +                                      void *opaque,
> +                                      virFreeCallback freecb)
> +{
> +    VIR_DEBUG("conn=%p, eventID=%d, cb=%p, opaque=%p, freecb=%p",
> +              conn, eventID, cb, opaque, freecb);
> +

this should have pool=%p as well

> +    virResetLastError();
> +
> +    virCheckConnectReturn(conn, -1);
> +    if (pool) {
> +        virCheckStoragePoolGoto(pool, error);
> +        if (pool->conn != conn) {
> +            virReportInvalidArg(pool,
> +                                _("storage pool'%s' in %s must match connection"),

Need a space after 'pool'. This is a long line, but since it's in an error
message I say just leave it for grepability

> +                                pool->name, __FUNCTION__);
> +            goto error;
> +        }
> +    }
> +    virCheckNonNullArgGoto(cb, error);
> +    virCheckNonNegativeArgGoto(eventID, error);
> +
> +    if (eventID >= VIR_STORAGE_POOL_EVENT_ID_LAST) {
> +        virReportInvalidArg(eventID,
> +                            _("eventID in %s must be less than %d"),
> +                            __FUNCTION__, VIR_STORAGE_POOL_EVENT_ID_LAST);
> +        goto error;
> +    }
> +
> +    if (conn->storageDriver && conn->storageDriver->connectStoragePoolEventRegisterAny) {

Long line, split it after the &&

> +        int ret;
> +        ret = conn->storageDriver->connectStoragePoolEventRegisterAny(conn, pool,
> +                                                                      eventID,
> +                                                                      cb, opaque,
> +                                                                      freecb);

If you put pool and opaque on their own lines this breaks some over 80 column
lines, but I don't know if that's better or worse

> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virReportUnsupportedError();
> + error:
> +    virDispatchError(conn);
> +    return -1;
> +}
> +
> +/**
> + * virConnectStoragePoolEventDeregisterAny:
> + * @conn: pointer to the connection
> + * @callbackID: the callback identifier
> + *
> + * Removes an event callback. The callbackID parameter should be the
> + * value obtained from a previous virConnectStoragePoolEventRegisterAny() method.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +virConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
> +                                        int callbackID)
> +{
> +    VIR_DEBUG("conn=%p, callbackID=%d", conn, callbackID);
> +
> +    virResetLastError();
> +
> +    virCheckConnectReturn(conn, -1);
> +    virCheckNonNegativeArgGoto(callbackID, error);
> +
> +    if (conn->storageDriver &&
> +        conn->storageDriver->connectStoragePoolEventDeregisterAny) {
> +        int ret;
> +        ret = conn->storageDriver->connectStoragePoolEventDeregisterAny(conn,
> +                                                                        callbackID);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virReportUnsupportedError();
> + error:
> +    virDispatchError(conn);
> +    return -1;
> +}
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 1e920d6..f5898ee 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -732,4 +732,11 @@ LIBVIRT_1.3.3 {
>          virDomainSetPerfEvents;
>  } LIBVIRT_1.2.19;
>  
> +
> +LIBVIRT_1.3.6 {
> +    global:
> +        virConnectStoragePoolEventRegisterAny;
> +        virConnectStoragePoolEventDeregisterAny;
> +} LIBVIRT_1.3.3;
> +
>  # .... define new API here using predicted next version number ....
> 

Aside from those issues this looks fine to me

Thanks,
Cole

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]