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

Re: [libvirt] [PATCH] event: improve public API docs



On Tue, Dec 31, 2013 at 08:21:29AM -0700, Eric Blake wrote:
> Since libvirt 0.9.3, the entire virevent.c file has been a public
> API, so improve the documentation in this file.  Also, fix a
> potential core dump - it could only be triggered by bogus use of
> the API and would only affect the caller (not libvirtd), but we
> might as well be nice.
> 
> * src/libvirt.c (virConnectDomainEventRegister)
> (virConnectDomainEventRegisterAny)
> (virConnectNetworkEventRegisterAny): Document event loop requirement.
> * src/util/virevent.c (virEventAddHandle, virEventRemoveHandle)
> (virEventAddTimeout, virEventRemoveTimeout): Likewise.
> (virEventUpdateHandle, virEventUpdateTimeout): Likewise, and avoid
> core dump if caller didn't register handler.
> (virEventRunDefaultImpl): Expand example, and set up code block in
> html docs.
> (virEventRegisterImpl, virEventRegisterDefaultImpl): Document more
> on the use of the event loop.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>  src/libvirt.c       | 24 ++++++++++------
>  src/util/virevent.c | 82 +++++++++++++++++++++++++++++++++++------------------
>  2 files changed, 70 insertions(+), 36 deletions(-)
> 
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 66841c8..f43718d 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -16121,11 +16121,13 @@ error:
>   * @freecb: optional function to deallocate opaque when not used anymore
>   *
>   * Adds a callback to receive notifications of domain lifecycle events
> - * occurring on a connection
> + * occurring on a connection.  This function requires that an event loop
> + * has been previously registered with virEventRegisterImpl() or
> + * virEventRegisterDefaultImpl().
>   *
>   * Use of this method is no longer recommended. Instead applications
>   * should try virConnectDomainEventRegisterAny() which has a more flexible
> - * API contract
> + * API contract.
>   *
>   * The virDomainPtr object handle passed into the callback upon delivery
>   * of an event is only valid for the duration of execution of the callback.
> @@ -16134,7 +16136,7 @@ error:
>   * The reference can be released once the object is no longer required
>   * by calling virDomainFree.
>   *
> - * Returns 0 on success, -1 on failure
> + * Returns 0 on success, -1 on failure.
>   */
>  int
>  virConnectDomainEventRegister(virConnectPtr conn,
> @@ -19064,10 +19066,12 @@ error:
>   * @freecb: optional function to deallocate opaque when not used anymore
>   *
>   * Adds a callback to receive notifications of arbitrary domain events
> - * occurring on a domain.
> + * occurring on a domain.  This function requires that an event loop
> + * has been previously registered with virEventRegisterImpl() or
> + * virEventRegisterDefaultImpl().
>   *
>   * If @dom is NULL, then events will be monitored for any domain. If @dom
> - * is non-NULL, then only the specific domain will be monitored
> + * is non-NULL, then only the specific domain will be monitored.
>   *
>   * Most types of event have a callback providing a custom set of parameters
>   * for the event. When registering an event, it is thus necessary to use
> @@ -19085,7 +19089,7 @@ error:
>   * for the callback. To unregister a callback, this callback ID should
>   * be passed to the virConnectDomainEventDeregisterAny() method.
>   *
> - * Returns a callback identifier on success, -1 on failure
> + * Returns a callback identifier on success, -1 on failure.
>   */
>  int
>  virConnectDomainEventRegisterAny(virConnectPtr conn,
> @@ -19183,10 +19187,12 @@ error:
>   * @freecb: optional function to deallocate opaque when not used anymore
>   *
>   * Adds a callback to receive notifications of arbitrary network events
> - * occurring on a network.
> + * occurring on a network.  This function requires that an event loop
> + * has been previously registered with virEventRegisterImpl() or
> + * virEventRegisterDefaultImpl().
>   *
>   * If @net is NULL, then events will be monitored for any network. If @net
> - * is non-NULL, then only the specific network will be monitored
> + * is non-NULL, then only the specific network will be monitored.
>   *
>   * Most types of event have a callback providing a custom set of parameters
>   * for the event. When registering an event, it is thus necessary to use
> @@ -19204,7 +19210,7 @@ error:
>   * for the callback. To unregister a callback, this callback ID should
>   * be passed to the virConnectNetworkEventDeregisterAny() method.
>   *
> - * Returns a callback identifier on success, -1 on failure
> + * Returns a callback identifier on success, -1 on failure.
>   */
>  int
>  virConnectNetworkEventRegisterAny(virConnectPtr conn,
> diff --git a/src/util/virevent.c b/src/util/virevent.c
> index fde29a2..0c94946 100644
> --- a/src/util/virevent.c
> +++ b/src/util/virevent.c
> @@ -37,6 +37,16 @@ static virEventAddTimeoutFunc addTimeoutImpl = NULL;
>  static virEventUpdateTimeoutFunc updateTimeoutImpl = NULL;
>  static virEventRemoveTimeoutFunc removeTimeoutImpl = NULL;
> 
> +
> +/*****************************************************
> + *
> + * Below this point are  *PUBLIC*  APIs for event
> + * loop integration with applications using libvirt.
> + * These API contracts cannot be changed.
> + *
> + *****************************************************/
> +
> +
>  /**
>   * virEventAddHandle:
>   *
> @@ -46,10 +56,12 @@ static virEventRemoveTimeoutFunc removeTimeoutImpl = NULL;
>   * @opaque: user data to pass to callback
>   * @ff: callback to free opaque when handle is removed
>   *
> - * Register a callback for monitoring file handle events.
> + * Register a callback for monitoring file handle events.  This function
> + * requires that an event loop has previously been registered with
> + * virEventRegisterImpl() or virEventRegisterDefaultImpl().
>   *
>   * Returns -1 if the file handle cannot be registered, otherwise a handle
> - * watch number to be used for updating and unregistering for events
> + * watch number to be used for updating and unregistering for events.
>   */
>  int
>  virEventAddHandle(int fd,
> @@ -70,14 +82,17 @@ virEventAddHandle(int fd,
>   * @watch: watch whose file handle to update
>   * @events: bitset of events to watch from virEventHandleType constants
>   *
> - * Change event set for a monitored file handle.
> + * Change event set for a monitored file handle.  This function
> + * requires that an event loop has previously been registered with
> + * virEventRegisterImpl() or virEventRegisterDefaultImpl().
>   *
> - * Will not fail if fd exists
> + * Will not fail if fd exists.
>   */
>  void
>  virEventUpdateHandle(int watch, int events)
>  {
> -    updateHandleImpl(watch, events);
> +    if (updateHandleImpl)
> +        updateHandleImpl(watch, events);
>  }
> 
>  /**
> @@ -85,7 +100,9 @@ virEventUpdateHandle(int watch, int events)
>   *
>   * @watch: watch whose file handle to remove
>   *
> - * Unregister a callback from a file handle.
> + * Unregister a callback from a file handle.  This function
> + * requires that an event loop has previously been registered with
> + * virEventRegisterImpl() or virEventRegisterDefaultImpl().
>   *
>   * Returns -1 if the file handle was not registered, 0 upon success.
>   */
> @@ -106,7 +123,9 @@ virEventRemoveHandle(int watch)
>   * @opaque: user data to pass to callback
>   * @ff: callback to free opaque when timeout is removed
>   *
> - * Register a callback for a timer event.
> + * Register a callback for a timer event.  This function
> + * requires that an event loop has previously been registered with
> + * virEventRegisterImpl() or virEventRegisterDefaultImpl().
>   *
>   * Setting timeout to -1 will disable the timer. Setting the timeout
>   * to zero will cause it to fire on every event loop iteration.
> @@ -132,17 +151,20 @@ virEventAddTimeout(int timeout,
>   * @timer: timer id to change
>   * @timeout: time between events in milliseconds
>   *
> - * Change frequency for a timer.
> + * Change frequency for a timer.  This function
> + * requires that an event loop has previously been registered with
> + * virEventRegisterImpl() or virEventRegisterDefaultImpl().
>   *
>   * Setting frequency to -1 will disable the timer. Setting the frequency
>   * to zero will cause it to fire on every event loop iteration.
>   *
> - * Will not fail if timer exists
> + * Will not fail if timer exists.
>   */
>  void
>  virEventUpdateTimeout(int timer, int timeout)
>  {
> -    updateTimeoutImpl(timer, timeout);
> +    if (updateTimeoutImpl)
> +        updateTimeoutImpl(timer, timeout);
>  }
> 
>  /**
> @@ -150,7 +172,9 @@ virEventUpdateTimeout(int timer, int timeout)
>   *
>   * @timer: the timer id to remove
>   *
> - * Unregister a callback for a timer.
> + * Unregister a callback for a timer.  This function
> + * requires that an event loop has previously been registered with
> + * virEventRegisterImpl() or virEventRegisterDefaultImpl().
>   *
>   * Returns -1 if the timer was not registered, 0 upon success.
>   */
> @@ -164,14 +188,6 @@ virEventRemoveTimeout(int timer)
>  }
> 
> 
> -/*****************************************************
> - *
> - * Below this point are 3  *PUBLIC*  APIs for event
> - * loop integration with applications using libvirt.
> - * These API contracts cannot be changed.
> - *
> - *****************************************************/
> -
>  /**
>   * virEventRegisterImpl:
>   * @addHandle: the callback to add fd handles
> @@ -186,9 +202,14 @@ virEventRemoveTimeout(int timer)
>   * to integrate with the libglib2 event loop, or libevent
>   * or the QT event loop.
>   *
> + * Use of the virEventAddHandle() and similar APIs require that the
> + * corresponding handler be registered.  Use of the
> + * virConnectDomainEventRegisterAny() and similar APIs requires that
> + * the three timeout handlers be registered.

s/be/to be/ or s/be/are/ maybe?

> + *
>   * If an application does not need to integrate with an
>   * existing event loop implementation, then the
> - * virEventRegisterDefaultImpl method can be used to setup
> + * virEventRegisterDefaultImpl() method can be used to setup
>   * the generic libvirt implementation.
>   */
>  void virEventRegisterImpl(virEventAddHandleFunc addHandle,
> @@ -220,9 +241,12 @@ void virEventRegisterImpl(virEventAddHandleFunc addHandle,
>   * not have a need to integrate with an external event
>   * loop impl.
>   *
> - * Once registered, the application has to invoke virEventRunDefaultImpl in
> + * Once registered, the application has to invoke virEventRunDefaultImpl() in
>   * a loop to process events.  Failure to do so may result in connections being
> - * closed unexpectedly as a result of keepalive timeout.
> + * closed unexpectedly as a result of keepalive timeout.  The default
> + * event loop fully supports handle and and timeout events, but only

s/and and/and/

> + * wakes up on events registered by libvirt API calls such as
> + * virEventAddHandle() or virConnectDomainEventRegisterAny().
>   *
>   * Returns 0 on success, -1 on failure.
>   */
> @@ -255,14 +279,18 @@ int virEventRegisterDefaultImpl(void)
>   *
>   * Run one iteration of the event loop. Applications
>   * will generally want to have a thread which invokes
> - * this method in an infinite loop
> + * this method in an infinite loop.  Furthermore, it is wise
> + * to set up a pipe-to-self handler (via virEventAddHandle())
> + * or a timeout (via virEventAddTimeout()) before calling this
> + * function, as it will block forever if there are no
> + * registered events.
>   *
> - *  static bool quit = false;
> + *   static bool quit = false;
>   *
> - *  while (!quit) {
> - *    if (virEventRunDefaultImpl() < 0)
> + *   while (!quit) {
> + *     if (virEventRunDefaultImpl() < 0)
>   *       ...print error...
> - *  }
> + *   }
>   *
>   * Returns 0 on success, -1 on failure.
>   */
> -- 
> 1.8.4.2
> 

ACK, I'll have a look at the fixup in a second.

Martin

Attachment: signature.asc
Description: Digital signature


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