Re: New GIOChannel patch

[Owen Taylor <otaylor redhat com>]

Ron Steinke <rsteinke w-link net> writes:

> > > > * Need to make a read-only-returns-whole-characters guarantee
> > > >   for reading for UTF-8, and validate incoming data.
> > > 
> > > Did this, excluding the encodings NULL (guaranteed to be binary safe)
> > > and GIOChannelEncodeRaw.
> >
> > So, we have for special encodings:
> >
> >  - "UTF-8" works just like it wasn't a special case, but doesn't
> >    involve opening a converter from UTF-8 to UTF-8. 
> >
> >  - NULL. No conversion, byte-by-byte.
> >
> >  - GIOChannelEncodeRaw. Like NULL but no buffering?
> >
> > The NULL / GIOChannelEncodeRaw distinction strikes me as something
> > that shouldn't be controlled by the encoding - since there
> > is no difference in encoding. Rather, there should be a 
> > separate g_io_channel_set_unbuffered(), or something.
> 
> Mabye we should just rename g_io_channel_set_encoding() to
> something more general.

Well, assuming we could come up with a decent name.

Argument for:

 * Buffering and encoding conversion do interact. You can't
   have encoding conversion without buffering.

   So mixing them together as a single setting does 
   follow the good rule of preventing mistakes by
   simply not having the potential for them in the interface.

Argument against:

 * I don't think people will really see the connection
   and will be confused if we mix the two together.
   It is easier, I think to present them as separate and
   then be told: if you call g_io_channel_set_unbuffered,
   you must use G_IO_CHANNEL_ENCODE_RAW as your encoding.
   
> > Well, that's handled right by:
> >
> >   /* Only return TRUE here if _all_ bits in watch->condition will be set
> >    */
> >   return ((watch->condition & buffer_condition) == watch->condition);
> >
> > Right? Actually, I don't think we should set the G_IO_OUT bit
> > unless we have space in the buffer ... if I'm trying to write
> > a 30 meg file to a socket, and am using G_IO_OUT, and I don't
> > want to create a 30 meg local bugger.
> 
> So the current approach seems to be correct. The only question I
> have left is if we need some sort of automatic flush watch.

I don't think an automatic flush watch is feasible ... we
generally have no idea whether the user is using a main
context, what main context it is using, or what the thread
contraints are.

However, you are right that there is a problem .... to flush output
through a buffer to a non-blocking location involves pushing data
along at two locations, not just one.  If we have space at the first
location, we should call g_io_channel_write_chars(), if we have space
at the second location, we should call g_io_channel_flush().

One idea is a special type of source: g_io_channel_add_flush_watch()
that gets called when the underlying file descriptor needs
flushing,

But this seems rather over-complex. An alternative would be to expose
the amount of data in the write buffer via an API function, and report
G_IO_OUT only when a write is allowed on the output stream. Then
people could write for their callback :

 if (g_io_channel_get_pending_write_bytes (channel) > HIGH_WATER_MARK)
   g_io_channel_flush (channel);

 while (g_io_channel_get_pending_write_bytes (channel) < LOW_WATER_MARK)
   {
     /* Write some more data into the buffer */
   }

> > > > * There is no reason (IMO) to expand the write buffer arbitrarily
> > > >   when writing characters - just flush out each chunk as 
> > > >   it converted. 
> > > 
> > > Writing before conversion is complete would cause problems in dealing
> > > with error conditions. Currently, if an error is detected in
> > > writing, the write buffer is returned to its previous condition
> > > and bytes_written is set to zero. If we start writing before we're
> > > sure there aren't any conversion errors, we can't do this.
> >
> > I think this is fine. We see the same behavior with out of space
> > errors - we write the part that fits, we get a short write,
> > we try again, we get a ENOSPACE error for the remainder,
> > and return that as a GError.
> >
> > If we wanted to preserve the "error returned only if nothing
> > written" aspect of write(), we'd have to make sure we only
> > ever did one write() per call of g_io_channel_write_chars()
> > which is neither easy to do nor desirable.
> 
> So, should I then return the number of characters actually written
> in *bytes_written even on error?

I don't think it would hurt to do this, though I'm not sure
when it would be useful either.

> >  * If it's a file, it's most likely a programming error
> >    and/or a misencoded internal stream, 
> >
> > I don't see any disaster in not reporting this error, and
> > it seems to me that making this an error could occasionally
> > be quite annoying.
> 
> Okay. I'll change it to a g_warning(), and clear the
> contents of the partial character buffer on flush.
> 
> This is now bug #57694.

The behavior that I would tend expect is:

 - Silently retain partial characters on g_io_channel_flush()

 - g_warning() in g_io_channel_read() if we, for a seekable
   channel have partial characters remaining after a flush.

I'm not sure if this is the behavior you are planning to
implement or not.

Regards,
                                        Owen