GNOME Bugzilla – Bug 679662
Simple way to read/write as much as possible from/to GBytes
Last modified: 2018-05-24 14:20:52 UTC
Discussed this with danw on irc. These functions have not yet been released in a stable release, so we can change the API and behavior of these. No uses in jhbuild outside of glib and bindings, as far as grep told me.
Created attachment 218382 [details] [review] gio: g_output_stream_write_bytes() writes all bytes * Make g_output_stream_write_bytes() and friends behave more like g_output_stream_write_all(). Write all bytes requested until error. * These functions become much less awkward to use with GBytes. * The GBytes buffer provides the perfect ref counted way to keep ahold of the memory while writing it all out.
Review of attachment 218382 [details] [review]: I'm really glad you caught this during development. I bet Jasper's code using it is buggy...
Which tells me that my grep wasn't right. I searched for the full C name, but I guess I should just search for 'write_bytes'...
(In reply to comment #1) > gio: g_output_stream_write_bytes() writes all bytes Either "Make g_output_stream_write_bytes() write all bytes" or "g_output_stream_write_bytes() should write all bytes" would be more consistent with the standard commit message style.
Comment on attachment 218382 [details] [review] gio: g_output_stream_write_bytes() writes all bytes >+ * If there is an error during the operation %FALSE is returned and @error >+ * is set to indicate the error status, @bytes_written is updated to contain >+ * the number of bytes written into the stream before the error occurred. Actually, this won't work for bindings; in bindings where GError gets turned into an exception, the return value and all out params are lost. (That applies to g_output_stream_write_all() too... but it's worse with write_bytes() since one of the points of write_bytes() is to be more bindings-friendly than write()/write_async().) I don't have any great ideas for fixing this... I guess we could have write_bytes() and write_bytes_all(), with the latter being non-bindings-friendly like write_all(). Actually, since write_bytes() has two use cases ((a) when you actually have a GBytes to write, and (b) to be more bindings-friendly), maybe the split would be better anyway, so bindings can still get the write()-like behavior too. All of this applies to the read_bytes() patch too.
> I don't have any great ideas for fixing this... I guess we could have > write_bytes() and write_bytes_all(), with the latter being > non-bindings-friendly like write_all(). Okay lets split it up. write_bytes_all() would be usable by bindings that don't care about partial writes. I can (skip) the bytes_written parameter so it's not available to bindings as it's not useful. FWIW, python and ruby don't seem to have a concept of partial writes at all. If an exception occurs during a f.write() then information about how many bytes may have been written is not represented. It seems to me that recovering from partial write on a failure using information on how many bytes were written is a really advanced feature. But I'll defer to your judgement here.
(In reply to comment #5) > (From update of attachment 218382 [details] [review]) > >+ * If there is an error during the operation %FALSE is returned and @error > >+ * is set to indicate the error status, @bytes_written is updated to contain > >+ * the number of bytes written into the stream before the error occurred. > > Actually, this won't work for bindings; in bindings where GError gets turned > into an exception, the return value and all out params are lost. (That applies > to g_output_stream_write_all() too... but it's worse with write_bytes() since > one of the points of write_bytes() is to be more bindings-friendly than > write()/write_async().) > > I don't have any great ideas for fixing this... I guess we could have > write_bytes() and write_bytes_all(), with the latter being > non-bindings-friendly like write_all(). In most of my C code lately even, I've been taking the stance that (out) arguments are set on success, and not touched if an exception is thrown. GLib is kind of inconsistent on this unfortunately. (In reply to comment #6) > It seems to me that recovering from > partial write on a failure using information on how many bytes were written is > a really advanced feature. Yeah...I think the main cases are like: 1) You are implementing something like a record-oriented filesystem journal, and if a write failure occurs, you need to know how many records you wrote so you can return success 2) A network protocol transferring data, where you can restart if you crashed by recording how much was written. (Really though, it's better to be stateless and have a system where you query the other end). One idea though is to have g_output_stream_get_last_write_size(), and is only set on error. It could be implemented with g_object_set_data() on the stream. Ugly, yes, but probably the least bad alternative.
(In reply to comment #7) > Ugly, yes, but probably the least bad alternative. I think I agree on both counts. It lets the "advanced" users do their thing, while keeping the base API clean for "simple" users.
Makes sense, but while trying to implement this concept realized that it doesn't really work for GInputStream. In that case you either need to keep a GBytes around (g_input_stream_get_last_partial_bytes()) which is extremely ugly .... So perhaps we do in fact need two APIs: * One which does a single read/write, and doesn't try to read/write as much as possible. Doesn't return error after data. Error should be returned on next read/write. Needs to be used in a (logical) loop of some sort. This would be read_bytes/write_bytes * The other which reads/writes all the data or error/eof. Doesn't allow caller to handle partial data read/written before failure. This would be read_all_bytes/write_all_bytes We already have read/read_all and write/write_all, so it's not that big of a stretch to have read_bytes/read_all_bytes and write_bytes/write_all_bytes.
*** Bug 679668 has been marked as a duplicate of this bug. ***
*** Bug 708838 has been marked as a duplicate of this bug. ***
(In reply to comment #5) > (From update of attachment 218382 [details] [review]) > >+ * If there is an error during the operation %FALSE is returned and @error > >+ * is set to indicate the error status, @bytes_written is updated to contain > >+ * the number of bytes written into the stream before the error occurred. > > Actually, this won't work for bindings; in bindings where GError gets turned > into an exception, the return value and all out params are lost. (That applies > to g_output_stream_write_all() too... but it's worse with write_bytes() since > one of the points of write_bytes() is to be more bindings-friendly than > write()/write_async().) So, from a quick scan of a jhbuild checkout, it looks like no one ever uses the "bytes_written" out argument of g_output_stream_write_all(). (Because whatever they were writing to gets deleted/disconnected/whatever after the error, so it doesn't matter.) So having a g_output_stream_write_bytes_all() which had the behavior that on failure, you wouldn't know how many bytes were written might be fine, as long as the docs were very explicit about it.
oh, someone made a comment yesterday about "that seems like more of a progress callback thing anyway". So another possibility would be to add a "wrote-bytes" signal to GOutputStream that gets emitted after every write, which would then let you figure out how many bytes had been written, even on failure, if you needed to know. (Ihat would solve the lack of progress reporting in g_output_stream_splice*() too...)
(In reply to comment #13) > oh, someone made a comment yesterday about "that seems like more of a progress > callback thing anyway". So another possibility would be to add a "wrote-bytes" > signal to GOutputStream that gets emitted after every write, which would then > let you figure out how many bytes had been written, even on failure, if you > needed to know. (Ihat would solve the lack of progress reporting in > g_output_stream_splice*() too...) I really don't like the idea of a signal. It's taking a core I/O loop that's otherwise relatively efficient (for GFileDescriptorBased -> GFileDescriptorBased) and causing it to involve gobject signals and mutexes and blah. And we don't even have a way to know whether there's something connected to the signal that actually cares. Yeah, you can't get progress from splice. If your app cares, do the read/write loop on your own?
(In reply to comment #14) > And we don't even have a way to know whether there's something connected to the > signal that actually cares. g_signal_has_handler_pending() ?
*** Bug 712390 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/570.