Re: [PATCH v4 4/7] fdstream: Emit stream abort callback even if poll() doesnt.

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

 



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@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
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]