Created attachment 210831 [details] [review]
0001-Make-GBufferedInputStream-implement-GSeekable.patch

Created attachment 210832 [details] [review]
0002-Make-GBufferedOutputStream-implement-GSeekable.patch

Created attachment 210833 [details] [review]
0003-Make-GDataOutputStream-implement-GSeekable.patch

Those patches don't include tests (yet). However the feedback is welcome.

Review of attachment 210831 [details] [review]:

We don't have a machine-readable way to mark an interface implementation as being new in a particular release, but you should at least add something like "As of glib 2.34, #GBufferedInputStream implements #GSeekable" to the main docs at the top of the file.

::: gio/gbufferedinputstream.c
@@ +887,3 @@
+  base_offset = g_seekable_tell (base_stream_seekable);
+
+  return base_offset - available;

That's wrong; the GBufferedInputStream's position is the same as its underlying stream's position. What portion of the stream happens to be currently buffered is irrelevant.

@@ +922,3 @@
+      if (offset <= priv->end - priv->pos && offset >= -priv->pos)
+	{
+	  priv->pos += offset;

likewise, this (and the rest of g_buffered_input_stream_seek) is mixing up the global stream position with the position within the buffer.

The buffering should have no visible effect on tell/seek operations: a series of reads, skips, tells, and seeks on the GBufferedInputStream should return exactly the same results as doing it on its base_stream.

Review of attachment 210832 [details] [review]:

same issues

Review of attachment 210833 [details] [review]:

Why does GDataOutputStream need its own implementation? It ought to be able to just inherit GBufferedOuputStream's.

(In reply to comment #5)
> Review of attachment 210831 [details] [review]:
> 
> We don't have a machine-readable way to mark an interface implementation as
> being new in a particular release, but you should at least add something like
> "As of glib 2.34, #GBufferedInputStream implements #GSeekable" to the main docs
> at the top of the file.
> 
> ::: gio/gbufferedinputstream.c
> @@ +887,3 @@
> +  base_offset = g_seekable_tell (base_stream_seekable);
> +
> +  return base_offset - available;
> 
> That's wrong; the GBufferedInputStream's position is the same as its underlying
> stream's position. What portion of the stream happens to be currently buffered
> is irrelevant.
> 

From what I understand it should return logical position of stream. For example if we have a buffer of size 4096, and reads of 2048:

+---------------+-------------------+--------------------+-------------------+
| After n reads | Base stream tell  | Position in buffer | Outer stream tell |
+---------------+-------------------+--------------------+-------------------+
|             0 |                 0 |                  0 |                 0 |
|             1 |              4096 |               2048 |              2048 |
|             2 |              4096 |               4096 |              4096 |
|             3 |              8192 |               2048 |              6144 |
|             4 |              8192 |               4096 |              8192 |
|           ... |               ... |                ... |               ... |
+---------------+-------------------+--------------------+-------------------+

Those position are not in sync - see http://git.gnome.org/browse/glib/tree/gio/gbufferedinputstream.c#n780 - if the data is in buffer then there is no call to underlaying stream (if there was what would be the point of buffering?).

> @@ +922,3 @@
> +      if (offset <= priv->end - priv->pos && offset >= -priv->pos)
> +    {
> +      priv->pos += offset;
> 
> likewise, this (and the rest of g_buffered_input_stream_seek) is mixing up the
> global stream position with the position within the buffer.
> 
> The buffering should have no visible effect on tell/seek operations: a series
> of reads, skips, tells, and seeks on the GBufferedInputStream should return
> exactly the same results as doing it on its base_stream.

Why? My believe was that GBufferedInputStream should be transparent in a sense that it should not be visible expect delay of some effects.

For example such code should pass (code is in pseudo-Vala):

var file_input = File.read ();
var input = new DataInputStream(file_input);
assert (input.tell () == 0);
input.read_int16 (NULL);
assert (input.tell () == 2);
// It is known that:
// 2 <= file_input.tell () <= input.buffer_size

EDIT: It seems that read_int16 reads only 2 elements but the generic read fills full buffer:

using GLib;

void main() {
	var file_input = File.new_for_path ("test.txt").read ();
	var input = new DataInputStream(file_input);
	stdout.printf("Offset: %d\n", (int)file_input.tell ());
	uint8[] buffer = new uint8[1024];
	ssize_t read = input.read (buffer);
	stdout.printf("Read: %d\n", (int)read);
	stdout.printf("Offset: %d\n", (int)file_input.tell ());
}

Output:
Offset: 0
Read: 1024
Offset: 4096

(In reply to comment #6)
> Review of attachment 210832 [details] [review]:
> 
> same issues

Same comments.

(In reply to comment #7)
> Review of attachment 210833 [details] [review]:
> 
> Why does GDataOutputStream need its own implementation? It ought to be able to
> just inherit GBufferedOuputStream's.

Well - GDataOuputStream does not inherit from GBufferedOutputStream.

In general the GSeekable might not be well-defined for GFilter{In,Out}putStream (conversions, buffering etc. would need to have special handling).

I agree with Maciej, the current position should return the "offset" where the next read operation starts returning data from, not what the underlying stream is positioned at.

Review of attachment 210832 [details] [review]:

::: gio/gbufferedoutputstream.c
@@ +600,3 @@
+	{
+	  memset (priv->buffer + priv->pos, 0, offset);
+	  priv->pos += offset;

This doesn't seem right. A seek ahead does not necessarily clear the skipped area (although that is what happens if you're at the end of a file *and* you then write something there). The stream might be replacing existing content, and the seek should not cause the skipped data to be cleared.

@@ +606,3 @@
+
+  flushed = flush_buffer (bstream, cancellable, error);
+  g_return_val_if_fail (flushed, FALSE);

This isn't right. g_return_val_if_fail should only be used to verify programmer assumtions. These checks get compiled out if you disable debug in your build.

@@ +637,3 @@
+
+  flushed = flush_buffer (bstream, cancellable, error);
+  g_return_val_if_fail (flushed, FALSE);

same here

sorry, i misread the patch. i thought it was always returning the current position within the internal buffer, completely ignoring the position in the underlying stream.

(In reply to comment #10)
> Review of attachment 210832 [details] [review]:
> 
> ::: gio/gbufferedoutputstream.c
> @@ +600,3 @@
> +    {
> +      memset (priv->buffer + priv->pos, 0, offset);
> +      priv->pos += offset;
> 
> This doesn't seem right. A seek ahead does not necessarily clear the skipped
> area (although that is what happens if you're at the end of a file *and* you
> then write something there). The stream might be replacing existing content,
> and the seek should not cause the skipped data to be cleared.
> 

I see. Then I will flush the buffer explicitly. There might be a problem with seek backwards as not the whole area will be flushed.

> @@ +606,3 @@
> +
> +  flushed = flush_buffer (bstream, cancellable, error);
> +  g_return_val_if_fail (flushed, FALSE);
> 
> This isn't right. g_return_val_if_fail should only be used to verify programmer
> assumtions. These checks get compiled out if you disable debug in your build.
> 

That's what I mean by rusty C/GObject :(. I'll post fixes to it today (and nearly ready tests for GDataOutputStream).

Regards

Created attachment 210904 [details] [review]
0001-Make-GBufferedInputStream-implement-GSeekable.patch

Created attachment 210905 [details] [review]
0002-Make-GBufferedOutputStream-implement-GSeekable.patch

Created attachment 210906 [details] [review]
0003-Make-GDataOutputStream-implement-GSeekable.patch

Changes:

 - Remove g_return_val_if_fail when appropiate
 - Bug causing infinite recursion in GDataOutputStream.seek fixed
 - Add tests for seek (but not truncate) for GDataOutputStream

These all look good to me, but can't go in until we've branched, current master is still the stable 2.32 release.

(In reply to comment #17)
> These all look good to me,

They are still not finished (tests may reveal some small bugs).

> but can't go in until we've branched, current master
> is still the stable 2.32 release.

That's what I expected.

(In reply to comment #18)
> (In reply to comment #17)
> > These all look good to me,
> 
> They are still not finished (tests may reveal some small bugs).

Yeah, these shouldn't go in without test cases.

Created attachment 211692 [details] [review]
0001-Make-GBufferedInputStream-implement-GSeekable.patch

Created attachment 211693 [details] [review]
0002-Make-GBufferedOutputStream-implement-GSeekable.patch

Created attachment 211694 [details] [review]
0003-Make-GDataOutputStream-implement-GSeekable.patch

Update of patches. Nearly all tests are added.

Created attachment 212582 [details] [review]
0001-Make-GBufferedInputStream-implement-GSeekable.patch

Created attachment 212583 [details] [review]
0002-Make-GBufferedOutputStream-implement-GSeekable.patch

Created attachment 212584 [details] [review]
0003-Make-GDataOutputStream-implement-GSeekable.patch

Should be OK.

Review of attachment 212582 [details] [review]:

::: gio/gbufferedinputstream.c
@@ +904,3 @@
+
+  base_stream = G_FILTER_INPUT_STREAM (seekable)->base_stream;
+  g_return_val_if_fail (G_IS_SEEKABLE (base_stream), 0);

Should not rely on g_return_val_if_fail here, also, should return G_IO_ERROR_NOT_SUPPORTED error.

Review of attachment 212583 [details] [review]:

::: gio/gbufferedoutputstream.c
@@ +542,3 @@
+
+  base_stream = G_FILTER_OUTPUT_STREAM (seekable)->base_stream;
+  g_return_val_if_fail (G_IS_SEEKABLE (base_stream), 0);

Shouldn't rely on g_return_val_if_fail here, must to a non-debug-only check.

@@ +573,3 @@
+
+  base_stream = G_FILTER_OUTPUT_STREAM (seekable)->base_stream;
+  g_return_val_if_fail (G_IS_SEEKABLE (base_stream), FALSE);

Same here. Also, should return G_IO_ERROR_NOT_SUPPORTED error.

@@ +604,3 @@
+  bstream = G_BUFFERED_OUTPUT_STREAM (seekable);
+  base_stream = G_FILTER_OUTPUT_STREAM (seekable)->base_stream;
+  g_return_val_if_fail (G_IS_SEEKABLE (base_stream), FALSE);

Same here.

Review of attachment 212584 [details] [review]:

::: gio/gdataoutputstream.c
@@ +562,3 @@
+
+  base_stream = G_FILTER_OUTPUT_STREAM (seekable)->base_stream;
+  g_return_val_if_fail (G_IS_SEEKABLE (base_stream), FALSE);

same here

@@ +586,3 @@
+
+  base_stream = G_FILTER_OUTPUT_STREAM (seekable)->base_stream;
+  g_return_val_if_fail (G_IS_SEEKABLE (base_stream), FALSE);

same here.

Review of attachment 212583 [details] [review]:

::: gio/tests/buffered-output-stream.c
@@ +237,3 @@
+  const gchar buffer[] = "abcdefghijklmnopqrstuvwxyz";
+
+  len = 8;

Len is undefined

@@ +243,3 @@
+  base_stream = G_MEMORY_OUTPUT_STREAM (g_memory_output_stream_new (stream_data, len, g_realloc, g_free));
+  stream = g_buffered_output_stream_new_sized (G_OUTPUT_STREAM (base_stream), 8);
+  seekable = G_SEEKABLE (stream);

No actual test here

@@ +256,3 @@
   g_test_add_func ("/buffered-output-stream/grow", test_grow);
+  g_test_add_func ("/buffered-output-stream/seek", test_seek);
+  g_test_add_func ("/buffered-output-stream/seek", test_truncate);

Should be "/buffered-output-stream/truncate" here

Pushed patches with above fixes to master.