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

2014-02-04 Thread Claudio Bley 
At Tue, 31 Dec 2013 08:21:29 -0700,
Eric Blake wrote:
 
 @@ -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)
  {

I just stumbled over the last sentence in this function's documentation.

What exactly is this meant to tell me? On first thought I figured this
to be a typo, actually meaning it will not fail if timer does not
exist (ie. just ignore the change request)?

Or, is it just to assure that the function will work (ie. change the
frequency of the timer) in any circumstances iff only the timer exists
in the first place?

But, then again, the function cannot fail since its return type is
void. So, I'd assume that the function will just always work anyway...

The same question arises for the virEventUpdateHandle function:

 - * Will not fail if fd exists
 + * Will not fail if fd exists.
  */
 void
 virEventUpdateHandle(int watch, int events)

TIA
Claudio

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

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

2014-02-04 Thread Daniel P. Berrange 
On Tue, Feb 04, 2014 at 10:07:58AM +0100, Claudio Bley wrote:
 At Tue, 31 Dec 2013 08:21:29 -0700,
 Eric Blake wrote:
  
  @@ -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)
   {
 
 I just stumbled over the last sentence in this function's documentation.
 
 What exactly is this meant to tell me? On first thought I figured this
 to be a typo, actually meaning it will not fail if timer does not
 exist (ie. just ignore the change request)?
 
 Or, is it just to assure that the function will work (ie. change the
 frequency of the timer) in any circumstances iff only the timer exists
 in the first place?
 
 But, then again, the function cannot fail since its return type is
 void. So, I'd assume that the function will just always work anyway...

Yes, it is basically saying that unless you have called it with a timer
id that is invalid, it is guaranteed to succeed to change the frequency.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

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

2014-02-04 Thread Daniel P. Berrange 
On Tue, Feb 04, 2014 at 04:32:10PM +0100, Claudio Bley wrote:
 At Tue, 4 Feb 2014 10:23:22 +,
 Daniel P. Berrange wrote:
  
  On Tue, Feb 04, 2014 at 10:07:58AM +0100, Claudio Bley wrote:
   At Tue, 31 Dec 2013 08:21:29 -0700,
   Eric Blake wrote:

@@ -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)
 {
   
   I just stumbled over the last sentence in this function's documentation.
   
   What exactly is this meant to tell me? On first thought I figured this
   to be a typo, actually meaning it will not fail if timer does not
   exist (ie. just ignore the change request)?
   
   Or, is it just to assure that the function will work (ie. change the
   frequency of the timer) in any circumstances iff only the timer exists
   in the first place?
   
   But, then again, the function cannot fail since its return type is
   void. So, I'd assume that the function will just always work anyway...
  
  Yes, it is basically saying that unless you have called it with a timer
  id that is invalid, it is guaranteed to succeed to change the frequency.
 
 Thank you, that clears it up.
 
 Would it make sense to reword or even remove that sentence? Because it
 implies that it will fail when the timer does NOT exist?

Sure, patches welcome :-)

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

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

2014-02-04 Thread Claudio Bley 
At Tue, 4 Feb 2014 10:23:22 +,
Daniel P. Berrange wrote:
 
 On Tue, Feb 04, 2014 at 10:07:58AM +0100, Claudio Bley wrote:
  At Tue, 31 Dec 2013 08:21:29 -0700,
  Eric Blake wrote:
   
   @@ -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)
{
  
  I just stumbled over the last sentence in this function's documentation.
  
  What exactly is this meant to tell me? On first thought I figured this
  to be a typo, actually meaning it will not fail if timer does not
  exist (ie. just ignore the change request)?
  
  Or, is it just to assure that the function will work (ie. change the
  frequency of the timer) in any circumstances iff only the timer exists
  in the first place?
  
  But, then again, the function cannot fail since its return type is
  void. So, I'd assume that the function will just always work anyway...
 
 Yes, it is basically saying that unless you have called it with a timer
 id that is invalid, it is guaranteed to succeed to change the frequency.

Thank you, that clears it up.

Would it make sense to reword or even remove that sentence? Because it
implies that it will fail when the timer does NOT exist?

Claudio

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

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

2014-01-02 Thread Martin Kletzander 
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 ebl...@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

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

2014-01-02 Thread Martin Kletzander 
On Wed, Jan 01, 2014 at 03:54:14PM -0700, Eric Blake wrote:
 On 12/31/2013 08:21 AM, 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 ebl...@redhat.com
  ---
   src/libvirt.c   | 24 ++--
   src/util/virevent.c | 82 
  +++--
   2 files changed, 70 insertions(+), 36 deletions(-)
 
 I plan on squashing this in, too.
 
 diff --git i/src/libvirt.c w/src/libvirt.c
 index 0752c3f..753c71f 100644
 --- i/src/libvirt.c
 +++ w/src/libvirt.c
 @@ -2,7 +2,7 @@
   * libvirt.c: Main interfaces for the libvirt library to handle
 virtualization
   *   domains from a process running in domain 0
   *
 - * Copyright (C) 2005-2006, 2008-2013 Red Hat, Inc.
 + * Copyright (C) 2005-2006, 2008-2014 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
 @@ -21454,17 +21454,18 @@ error:
   * @interval: number of seconds of inactivity before a keepalive
 message is sent
   * @count: number of messages that can be sent in a row
   *
 - * Start sending keepalive messages after interval second of inactivity and
 + * Start sending keepalive messages after @interval seconds of
 inactivity and
   * consider the connection to be broken when no response is received after
 - * count keepalive messages sent in a row.  In other words, sending
 count + 1
 - * keepalive message results in closing the connection.  When interval
 is = 0,
 - * no keepalive messages will be sent.  When count is 0, the connection
 will be
 - * automatically closed after interval seconds of inactivity without
 sending
 - * any keepalive messages.
 - *
 - * Note: client has to implement and run event loop to be able to use
 keepalive
 - * messages.  Failure to do so may result in connections being closed
 - * unexpectedly.
 + * @count keepalive messages sent in a row.  In other words, sending
 count + 1
 + * keepalive message results in closing the connection.  When @interval is
 + * = 0, no keepalive messages will be sent.  When @count is 0, the
 connection
 + * will be automatically closed after interval seconds of inactivity
 without

s/interval/@interval/

This fixup came through word-wrapped.  Check your e-mail client
settings when pasting patches ;)

ACK to squash it in the previous patch.

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-01-02 Thread Eric Blake 
On 01/02/2014 06:15 AM, Martin Kletzander wrote:
 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.


   *
 + * 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?

I went with s/be/are/


 + * 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/

Yeah, 'make syntax-check' is nice :)

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

I've cleaned up according to your suggestions (here, and in the
followup), and pushed.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-01-01 Thread Eric Blake 
On 12/31/2013 08:21 AM, 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 ebl...@redhat.com
 ---
  src/libvirt.c   | 24 ++--
  src/util/virevent.c | 82 
 +++--
  2 files changed, 70 insertions(+), 36 deletions(-)

I plan on squashing this in, too.

diff --git i/src/libvirt.c w/src/libvirt.c
index 0752c3f..753c71f 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -2,7 +2,7 @@
  * libvirt.c: Main interfaces for the libvirt library to handle
virtualization
  *   domains from a process running in domain 0
  *
- * Copyright (C) 2005-2006, 2008-2013 Red Hat, Inc.
+ * Copyright (C) 2005-2006, 2008-2014 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
@@ -21454,17 +21454,18 @@ error:
  * @interval: number of seconds of inactivity before a keepalive
message is sent
  * @count: number of messages that can be sent in a row
  *
- * Start sending keepalive messages after interval second of inactivity and
+ * Start sending keepalive messages after @interval seconds of
inactivity and
  * consider the connection to be broken when no response is received after
- * count keepalive messages sent in a row.  In other words, sending
count + 1
- * keepalive message results in closing the connection.  When interval
is = 0,
- * no keepalive messages will be sent.  When count is 0, the connection
will be
- * automatically closed after interval seconds of inactivity without
sending
- * any keepalive messages.
- *
- * Note: client has to implement and run event loop to be able to use
keepalive
- * messages.  Failure to do so may result in connections being closed
- * unexpectedly.
+ * @count keepalive messages sent in a row.  In other words, sending
count + 1
+ * keepalive message results in closing the connection.  When @interval is
+ * = 0, no keepalive messages will be sent.  When @count is 0, the
connection
+ * will be automatically closed after interval seconds of inactivity
without
+ * sending any keepalive messages.
+ *
+ * Note: The client has to implement and run an event loop with
+ * virEventRegisterImpl() or virEventRegisterDefaultImpl() to be able to
+ * use keepalive messages.  Failure to do so may result in connections
+ * being closed unexpectedly.
  *
  * Note: This API function controls only keepalive messages sent by the
client.
  * If the server is configured to use keepalive you still need to run
the event
diff --git i/src/util/virevent.c w/src/util/virevent.c
index 0c94946..f3cebe0 100644
--- i/src/util/virevent.c
+++ w/src/util/virevent.c
@@ -1,7 +1,7 @@
 /*
  * virevent.c: event loop for monitoring file handles
  *
- * Copyright (C) 2007, 2011, 2013 Red Hat, Inc.
+ * Copyright (C) 2007, 2011, 2013-2014 Red Hat, Inc.
  * Copyright (C) 2007 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -205,7 +205,11 @@ virEventRemoveTimeout(int timer)
  * 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.
+ * the three timeout handlers be registered.  Likewise, the three
+ * timeout handlers must be registered if the remote server has been
+ * configured to send keepalive messages, or if the client intends
+ * to call virConnectSetKeepAlive(), to avoid either side from
+ * unexpectedly closing the connection due to inactivity.
  *
  * If an application does not need to integrate with an
  * existing event loop implementation, then the

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list