[Spice-devel] [PATCH spice-gtk 1/5] Add GIOStream-based pipe

[Marc-André Lureau]

Hi

----- Original Message -----
> > Because that field is for peer closing. I can rename it peer_closed
> > perhaps? (peer->peer_closed...)
> 
> You lost me here. Do you mean PipeInputStream::closed means that the
> peer PipeOutputStream has been closed? If that's what you mean, then it
> definitely should be peer_closed.

I don't mind, for me the field means the pipe is closing, either side. However, since it's internal to the pipe implementation and is only set by the peer, I guess it could be closed peer_closed too. That really doesn't change much of the meaning of this code anyway.


> > Yes, I am not sure what that means in practice. There are also default
> > _async fallback for other methods, this might be outdated comment?
> 
> Maybe in some places, gio is only checking if one of the async method is
> implemented, and then directly calls random other async methods. Is this
> _async implementation strictly needed anyway?

Probably not strictly required. I remember I based the code on gmemoryoutputstream and though it was a good idea to close sync and avoid threads (there are no pending IO handling either in giopipe)

However, note that close_async() has a default implementation, so it doesn't change much the checking for async implementation when overriding it.

> > >> +static gssize
> > >> +pipe_output_stream_write (GOutputStream  *stream,
> > >> +                          const void     *buffer,
> > >> +                          gsize           count,
> > >> +                          GCancellable   *cancellable,
> > >> +                          GError        **error)
> > >> +{
> > >> +    PipeOutputStream *self = PIPE_OUTPUT_STREAM(stream);
> > >> +    PipeInputStream *peer = self->peer;
> > >> +
> > >> +    //g_debug("write %p :%"G_GSIZE_FORMAT, stream, count);
> > >> +    if (g_output_stream_is_closed (stream) || self->closed) {
> > >> +        g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_CLOSED,
> > >> +                             "Stream is already closed");
> > >> +        return -1;
> > >> +    }
> > >> +
> > >> +    /* this abuses pollable stream, writing sync would likely lead to
> > >> +       crashes, since the buffer pointer would become invalid, a
> > >> +       generic solution would need a copy..
> > >> +    */
> > >> +    g_return_val_if_fail(self->buffer == buffer || self->buffer ==
> > >> NULL, -1);
> > >> +    self->buffer = buffer;
> > >> +    self->count = count;
> > >> +
> > >> +    pipe_input_stream_check_source(self->peer);
> > >
> > > This call will trigger a sync call to pipe_input_stream_read() if I
> > > followed everything properly? A comment mentioning that might make the
> > > code easier to follow.
> > 
> > It's not sync, it will "schedule peer source". As said in the comment,
> > this abuses pollable stream, since it keeps a pointer to the buffer
> > and assumes the function will be resumed with the same data (there are
> > preconditions checks for that)
> 
> Yup, the comment was not crystal clear, bit more understandable after looking
> at the next patches...
> 
> Christophe
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>