Re: New GIOChannel patch

[Ron Steinke <rsteinke w-link net>]

> From: 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.

I think you're right about two different functions being clearer.

> > > > > * 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.

It will mostly be useful for EAGAIN, where the program needs to
know where to take up writing.

> > >  * 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.

This is slightly different from what I was doing. I'll change it
to match. This also requires warnings in seek() and close()
paralleling the one in read().

I'm going to go ahead and commit the patch at this point. Besides
adding something to the changelog, is there anything else I need
to do?

Ron