Remove the "buf = NULL" => skip requirement from pb_istream_t callbacks.

Rationale: it's easy to implement the callback wrong. Doing so introduces
io errors when unknown fields are present in the input. If code is not
tested with unknown fields, these bugs can remain hidden for long time.

Added a special case for the memory buffer stream, where it gives a small
speed benefit.

Added testcase for skipping fields with test_decode2 implementation.

Update issue 37
Status: FixedInGit

diff --git a/docs/concepts.rst b/docs/concepts.rst
index d326114..355af25 100644 (file)
--- a/docs/concepts.rst
+++ b/docs/concepts.rst
@@ -92,9 +92,8 @@ Writing to stdout::
 
 Input streams
 -------------
-For input streams, there are a few extra rules:
+For input streams, there is one extra rule:
 
-#) If buf is NULL, read from stream but don't store the data. This is used to skip unknown input.
 #) You don't need to know the length of the message in advance. After getting EOF error when reading, set bytes_left to 0 and return false. Pb_decode will detect this and if the EOF was in a proper position, it will return true.
 
 Here is the structure::

diff --git a/example/common.c b/example/common.c
index b27ccae..04a5aa8 100644 (file)
--- a/example/common.c
+++ b/example/common.c
@@ -19,15 +19,6 @@ static bool read_callback(pb_istream_t *stream, uint8_t *buf, size_t count)
     int fd = (intptr_t)stream->state;
     int result;
     
-    if (buf == NULL)
-    {
-        /* Well, this is a really inefficient way to skip input. */
-        /* It is only used when there are unknown fields. */
-        char dummy;
-        while (count-- && recv(fd, &dummy, 1, 0) == 1);
-        return count == 0;
-    }
-    
     result = recv(fd, buf, count, MSG_WAITALL);
     
     if (result == 0)

diff --git a/pb_decode.c b/pb_decode.c
index 86dec4b..8e01fd7 100644 (file)
--- a/pb_decode.c
+++ b/pb_decode.c
@@ -36,26 +36,41 @@ static const pb_decoder_t PB_DECODERS[PB_LTYPES_COUNT] = {
  * pb_istream *
  **************/
 
-bool checkreturn pb_read(pb_istream_t *stream, uint8_t *buf, size_t count)
+static bool checkreturn buf_read(pb_istream_t *stream, uint8_t *buf, size_t count)
 {
-    if (stream->bytes_left < count)
-        PB_RETURN_ERROR(stream, "end-of-stream");
+    uint8_t *source = (uint8_t*)stream->state;
     
-    if (!stream->callback(stream, buf, count))
-        PB_RETURN_ERROR(stream, "io error");
+    if (buf != NULL)
+        memcpy(buf, source, count);
     
-    stream->bytes_left -= count;
+    stream->state = source + count;
     return true;
 }
 
-static bool checkreturn buf_read(pb_istream_t *stream, uint8_t *buf, size_t count)
+bool checkreturn pb_read(pb_istream_t *stream, uint8_t *buf, size_t count)
 {
-    uint8_t *source = (uint8_t*)stream->state;
+       if (buf == NULL && stream->callback != buf_read)
+       {
+               /* Skip input bytes */
+               uint8_t tmp[16];
+               while (count > 16)
+               {
+                       if (!pb_read(stream, tmp, 16))
+                               return false;
+                       
+                       count -= 16;
+               }
+               
+               return pb_read(stream, tmp, count);
+       }
+
+    if (stream->bytes_left < count)
+        PB_RETURN_ERROR(stream, "end-of-stream");
     
-    if (buf != NULL)
-        memcpy(buf, source, count);
+    if (!stream->callback(stream, buf, count))
+        PB_RETURN_ERROR(stream, "io error");
     
-    stream->state = source + count;
+    stream->bytes_left -= count;
     return true;
 }
 

diff --git a/pb_decode.h b/pb_decode.h
index 483665e..2be9205 100644 (file)
--- a/pb_decode.h
+++ b/pb_decode.h
@@ -19,12 +19,10 @@
  * Rules for callback:
  * 1) Return false on IO errors. This will cause decoding to abort.
  * 
- * 2) If buf is NULL, read but don't store bytes ("skip input").
- * 
- * 3) You can use state to store your own data (e.g. buffer pointer),
+ * 2) You can use state to store your own data (e.g. buffer pointer),
  * and rely on pb_read to verify that no-body reads past bytes_left.
  * 
- * 4) Your callback may be used with substreams, in which case bytes_left
+ * 3) Your callback may be used with substreams, in which case bytes_left
  * is different than from the main stream. Don't use bytes_left to compute
  * any pointers.
  */

diff --git a/tests/Makefile b/tests/Makefile
index 9b02817..73efbe6 100644 (file)
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -70,6 +70,9 @@ run_unittests: decode_unittests encode_unittests test_cxxcompile test_encode1 te
        [ "`./test_encode2 | ./test_decode2`" = \
        "`./test_encode2 | protoc --decode=Person -I. -I../generator -I/usr/include person.proto`" ]
        
+       [ "`./test_decode2 < person_with_extra_field.pb`" = \
+       "`cat person_with_extra_field.txt`" ]
+       
        [ "`./test_encode_callbacks | ./test_decode_callbacks`" = \
        "`./test_encode_callbacks | protoc --decode=TestMessage callbacks.proto`" ]
 

diff --git a/tests/person_with_extra_field.pb b/tests/person_with_extra_field.pb
new file mode 100644 (file)
index 0000000..00d153c
Binary files /dev/null and b/tests/person_with_extra_field.pb differ

diff --git a/tests/person_with_extra_field.txt b/tests/person_with_extra_field.txt
new file mode 100644 (file)
index 0000000..fae9f87
--- /dev/null
+++ b/tests/person_with_extra_field.txt
@@ -0,0 +1,3 @@
+name: "Test Person 99"
+id: 99
+email: "test@person.com"

diff --git a/tests/test_decode2.c b/tests/test_decode2.c
index 762b2b3..2142977 100644 (file)
--- a/tests/test_decode2.c
+++ b/tests/test_decode2.c
@@ -59,13 +59,6 @@ bool callback(pb_istream_t *stream, uint8_t *buf, size_t count)
     FILE *file = (FILE*)stream->state;
     bool status;
     
-    if (buf == NULL)
-    {
-       /* Skipping data */
-        while (count-- && fgetc(file) != EOF);
-        return count == 0;
-    }
-    
     status = (fread(buf, 1, count, file) == count);
     
     if (feof(file))