On 02/06/2012 06:50 AM, Peter Krempa wrote:
> This patch adds another callback to a FDstream object. The original
> callback is used by the daemon stream driver to handle events.
>
> This callback is called if and only if the stream is about to be closed.
> This might be used to handle cleanup steps after a fdstream exits. This
> will be used later on in ensuring mutualy exclusive access to consoles.
s/mutualy/mutually/
>
> * src/fdstream.c:
> - emit the callback, when stream is being closed
> - add data structures needed to handle the callback
> - add function to register callback
> * src/fdstream.h:
> - define function prototypes for the callback
> ---
> src/fdstream.c | 39 +++++++++++++++++++++++++++++++++++++--
> src/fdstream.h | 11 +++++++++++
> 2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/src/fdstream.c b/src/fdstream.c
> index 35f5135..192a7c4 100644
> --- a/src/fdstream.c
> +++ b/src/fdstream.c
> @@ -67,6 +67,12 @@ struct virFDStreamData {
> bool abortCallbackCalled;
> bool abortCallbackDispatching;
>
> + /* internal callback, as the regular one (from generic streams) gets
> + * eaten up by the server stream driver */
> + virFDStreamInternalCloseCb icbCb;
> + virFDStreamInternalCloseCbFreeOpaque icbFreeOpaque;
> + void *icbOpaque;
> +
> virMutex lock;
> };
>
> @@ -232,6 +238,7 @@ cleanup:
> return ret;
> }
>
> +
> static int
This whitespace change can be dropped, or moved to the patch that
introduces virFDStreamCloseInt.
> virFDStreamCloseInt(virStreamPtr st, bool streamAbort)
> {
> @@ -251,7 +258,7 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort)
> fdst->cb &&
> (fdst->events & (VIR_STREAM_EVENT_READABLE |
> VIR_STREAM_EVENT_WRITABLE))) {
> - /* don't enter this function accidentaly from the callback again */
> + /* don't enter this function accidentally from the callback again */
This hunk belongs in an earlier patch.
> if (fdst->abortCallbackCalled) {
> virMutexUnlock(&fdst->lock);
> return 0;
> @@ -261,7 +268,7 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort)
> fdst->abortCallbackDispatching = true;
> virMutexUnlock(&fdst->lock);
>
> - /* call failure callback, poll does report nothing on closed fd */
> + /* call failure callback, poll reports nothing on closed fd */
Same here.
> (fdst->cb)(st, VIR_STREAM_EVENT_ERROR, fdst->opaque);
>
> virMutexLock(&fdst->lock);
> @@ -306,6 +313,14 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort)
>
> st->privateData = NULL;
>
> + /* call the internal stream closing callback */
> + if (fdst->icbCb) {
> + /* the mutex is not accessible anymore, as private data are null */
s/are/is/
> + (fdst->icbCb)(st, fdst->icbOpaque);
> + if (fdst->icbFreeOpaque && fdst->icbOpaque)
Why are we refusing to call the icbFreeOpaque callback if icbOpaque is
NULL? You should never make decisions based on an opaque pointer,
because NULL might mean something to the callback. This should be:
if (fdst->icbFreeOpaque)
> + (fdst->icbFreeOpaque)(fdst->icbOpaque);
> + }
> +
> if (fdst->dispatching) {
> fdst->closed = true;
> virMutexUnlock(&fdst->lock);
> @@ -680,3 +695,23 @@ int virFDStreamCreateFile(virStreamPtr st,
> offset, length,
> oflags | O_CREAT, mode);
> }
> +
> +int virFDStreamSetInternalCloseCb(virStreamPtr st,
> + virFDStreamInternalCloseCb cb,
> + void *opaque,
> + virFDStreamInternalCloseCbFreeOpaque fcb)
> +{
> + struct virFDStreamData *fdst = st->privateData;
> +
> + virMutexLock(&fdst->lock);
> +
> + if (fdst->icbOpaque && fdst->icbFreeOpaque)
Same here.
I'm not quite sure how you are planning on using this extra callback; I
guess I need to continue on in the review; but assuming the use of this
looks reasonable, I think you can fix the problems I pointed out and get
my ACK.
--
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