On 02/06/2012 06:50 AM, Peter Krempa wrote:
This patch causes the fdstream driver to call the stream event callback if virStreamAbort() is issued on a stream using this driver. This prohibited to abort streams from the daemon, as the daemon remote handler installs a callback to watch for stream errors as the only mean of detecting changes in the stream.
That sentence didn't parse well for me; are you trying to say: A remote handler for a stream can only detect changes via stream events, so this event callback is necessary in order to enable a daemon to abort a stream in such a way that the client will see the change.
* src/fdstream.c: - modify close function to call stream event callback --- src/fdstream.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/src/fdstream.c b/src/fdstream.c index 841f979..35f5135 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -1,5 +1,5 @@ /* - * fdstream.h: generic streams impl for file descriptors + * fdstream.c: generic streams impl for file descriptors
Cute typo fix...
* * Copyright (C) 2009-2011 Red Hat, Inc.
but it made me notice this. It's now 2012. If you use emacs, you can add this to your ~/.emacs to auto-update copyright in any file you touch; here's what I use: (require 'copyright) (defun my-copyright-update (&optional arg) "My improvements to `copyright-update'." (interactive "*P") (and (not (eq major-mode 'fundamental-mode)) (copyright-update arg)) nil) (add-hook 'before-save-hook 'my-copyright-update) [hmm - should we follow the lead of GNU programs that just globally update the copyright year on all files when a new year rolls around? but that's a question to ask Red Hat legal]
@@ -120,6 +126,7 @@ static int virFDStreamUpdateCallback(virStreamPtr stream, int events) }
virEventUpdateHandle(fdst->watch, events); + fdst->events = events;
On update, you leave fdst->abortCallbackCalled unchanged,
ret = 0;
@@ -214,6 +221,8 @@ virFDStreamAddCallback(virStreamPtr st, fdst->cb = cb; fdst->opaque = opaque; fdst->ff = ff; + fdst->events = events; + fdst->abortCallbackCalled = false;
but on Add, you always clear it. Any significance to this difference?
+ /* aborting the stream, ensure the callback is called if it's + * registered for stream error event */ + if (streamAbort && + fdst->cb && + (fdst->events & (VIR_STREAM_EVENT_READABLE | + VIR_STREAM_EVENT_WRITABLE))) { + /* don't enter this function accidentaly from the callback again */
s/accidentaly/accidentally/
+ if (fdst->abortCallbackCalled) { + virMutexUnlock(&fdst->lock); + return 0; + } + + fdst->abortCallbackCalled = true; + fdst->abortCallbackDispatching = true; + virMutexUnlock(&fdst->lock); + + /* call failure callback, poll does report nothing on closed fd */
s/does report/reports/
+ (fdst->cb)(st, VIR_STREAM_EVENT_ERROR, fdst->opaque);
You need to cache fdst->cb and fdst->opaque before dropping locks, since otherwise you could have a race with someone else updating the callback to a different value. I'd still feel more comfortable with a review from Dan, but since that seems to be long in coming, you have my ACK if you can fix the problems pointed out above. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org