Re: [libvirt] [PATCH] event: improve public API docs
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
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
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
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
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
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
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
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
[libvirt] [PATCH] event: improve public API docs
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 *PUBLIC* APIs for event + * loop integration with applications using libvirt. + * These API contracts cannot be changed. + * +