Implement error messages in the decoder side.

Update issue 7
Status: Started

12 files changed, 52 insertions, 32 deletions

diff --git a/docs/concepts.rst b/docs/concepts.rst
index 1d0fe08f..122d29cf 100644
--- a/docs/concepts.rst
+++ b/docs/concepts.rst
@@ -258,18 +258,14 @@ generates this field description array for the structure *Person_PhoneNumber*::
 Return values and error handling
 ================================
 
-Most functions in nanopb return bool: *true* means success, *false* means failure. If this is enough for you, skip this section.
+Most functions in nanopb return bool: *true* means success, *false* means failure. There is also some support for error messages for debugging purposes: the error messages go in *stream->errmsg*.
 
-For simplicity, nanopb doesn't define it's own error codes. This might be added if there is a compelling need for it. You can however deduce something about the error causes:
+The error messages help in guessing what is the underlying cause of the error. The most common error conditions are:
 
 1) Running out of memory. Because everything is allocated from the stack, nanopb can't detect this itself. Encoding or decoding the same type of a message always takes the same amount of stack space. Therefore, if it works once, it works always.
 2) Invalid field description. These are usually stored as constants, so if it works under the debugger, it always does.
-3) IO errors in your own stream callbacks. Because encoding/decoding stops at the first error, you can overwrite the *state* field in the struct and store your own error code there.
-4) Errors that happen in your callback functions. You can use the state field in the callback structure.
+3) IO errors in your own stream callbacks.
+4) Errors that happen in your callback functions.
 5) Exceeding the max_size or bytes_left of a stream.
 6) Exceeding the max_size of a string or array field
-7) Invalid protocol buffers binary message. It's not like you could recover from it anyway, so a simple failure should be enough.
-
-In my opinion, it is enough that 1. and 2. can be resolved using a debugger.
-
-However, you may be interested which of the remaining conditions caused the error. For 3. and 4., you can set and check the state. If you have to detect 5. and 6., you should convert the fields to callback type. Any remaining problem is of type 7.
+7) Invalid protocol buffers binary message.
diff --git a/docs/reference.rst b/docs/reference.rst
index 3a6e11a4..ec9aec5d 100644
--- a/docs/reference.rst
+++ b/docs/reference.rst
@@ -20,6 +20,8 @@ PB_FIELD_16BIT                 Add support for tag numbers > 255 and fields larg
                                Increases code size 3 bytes per each field. Compiler error will tell if you need this.
 PB_FIELD_32BIT                 Add support for tag numbers > 65535 and fields larger than 65535 bytes or 65535 array entries.
                                Increases code size 9 bytes per each field. Compiler error will tell if you need this.
+PB_NO_ERRMSG                   Disables the support for error messages; only error information is the true/false return value.
+                               Decreases the code size by a few hundred bytes.
 ============================  ================================================================================================
 
 The PB_MAX_REQUIRED_FIELDS, PB_FIELD_16BIT and PB_FIELD_32BIT settings allow raising some datatype limits to suit larger messages.
@@ -431,7 +433,7 @@ Decode the length for a field with wire type *PB_WT_STRING* and create a substre
 This function uses `pb_decode_varint`_ to read an integer from the stream. This is interpreted as a number of bytes, and the substream is set up so that its `bytes_left` is initially the same as the length, and its callback function and state the same as the parent stream.
 
 pb_close_string_substream
-------------------------
+-------------------------
 Close the substream created with `pb_make_string_substream`_. ::
 
     void pb_close_string_substream(pb_istream_t *stream, pb_istream_t *substream);
diff --git a/example/client.c b/example/client.c
index 9ad9c8c9..edc83948 100644
--- a/example/client.c
+++ b/example/client.c
@@ -72,7 +72,7 @@ bool listdir(int fd, char *path)
     
     if (!pb_decode(&input, ListFilesResponse_fields, &response))
     {
-        fprintf(stderr, "Decoding failed.\n");
+        fprintf(stderr, "Decode failed: %s\n", PB_GET_ERROR(&input));
         return false;
     }
     
diff --git a/example/server.c b/example/server.c
index 9f27906d..346e9fb9 100644
--- a/example/server.c
+++ b/example/server.c
@@ -55,7 +55,7 @@ void handle_connection(int connfd)
     
     if (!pb_decode(&input, ListFilesRequest_fields, &request))
     {
-        printf("Decoding failed.\n");
+        printf("Decode failed: %s\n", PB_GET_ERROR(&input));
         return;
     }
     
diff --git a/example_unions/decode.c b/example_unions/decode.c
index d40cd8ce..b9f4af55 100644
--- a/example_unions/decode.c
+++ b/example_unions/decode.c
@@ -85,7 +85,7 @@ int main()
     
     if (!status)
     {
-        printf("Decoding failed.\n");
+        printf("Decode failed: %s\n", PB_GET_ERROR(&stream));
         return 1;
     }
     
diff --git a/pb.h b/pb.h
index 4983d069..85f4421c 100644
--- a/pb.h
+++ b/pb.h
@@ -196,5 +196,23 @@ typedef enum {
 #define pb_delta_end(st, m1, m2) (offsetof(st, m1) - offsetof(st, m2) - pb_membersize(st, m2))
 #define PB_LAST_FIELD {0,(pb_type_t) 0,0,0}
 
+/* These macros are used for giving out error messages.
+ * They are mostly a debugging aid; the main error information
+ * is the true/false return value from functions.
+ * Some code space can be saved by disabling the error
+ * messages if not used.
+ */
+#ifdef PB_NO_ERRMSG
+#define PB_RETURN_ERROR(stream,msg) return false
+#define PB_GET_ERROR(stream) "(errmsg disabled)"
+#else
+#define PB_RETURN_ERROR(stream,msg) \
+    do {\
+        if ((stream)->errmsg == NULL) \
+            (stream)->errmsg = (msg); \
+        return false; \
+    } while(0)
+#define PB_GET_ERROR(stream) ((stream)->errmsg ? (stream)->errmsg : "(none)")
+#endif
 
 #endif
diff --git a/pb_decode.c b/pb_decode.c
index 8f72f776..9d65504c 100644
--- a/pb_decode.c
+++ b/pb_decode.c
@@ -39,10 +39,10 @@ static const pb_decoder_t PB_DECODERS[PB_LTYPES_COUNT] = {
 bool checkreturn pb_read(pb_istream_t *stream, uint8_t *buf, size_t count)
 {
     if (stream->bytes_left < count)
-        return false;
+        PB_RETURN_ERROR(stream, "end-of-stream");
     
     if (!stream->callback(stream, buf, count))
-        return false;
+        PB_RETURN_ERROR(stream, "io error");
     
     stream->bytes_left -= count;
     return true;
@@ -95,7 +95,7 @@ bool checkreturn pb_decode_varint(pb_istream_t *stream, uint64_t *dest)
             return true;
     }
     
-    return false;
+    PB_RETURN_ERROR(stream, "varint overflow");
 }
 
 bool checkreturn pb_skip_varint(pb_istream_t *stream)
@@ -152,7 +152,7 @@ bool checkreturn pb_skip_field(pb_istream_t *stream, pb_wire_type_t wire_type)
         case PB_WT_64BIT: return pb_read(stream, NULL, 8);
         case PB_WT_STRING: return pb_skip_string(stream);
         case PB_WT_32BIT: return pb_read(stream, NULL, 4);
-        default: return false;
+        default: PB_RETURN_ERROR(stream, "invalid wire_type");
     }
 }
 
@@ -182,7 +182,7 @@ static bool checkreturn read_raw_value(pb_istream_t *stream, pb_wire_type_t wire
             *size = 4;
             return pb_read(stream, buf, 4);
         
-        default: return false;
+        default: PB_RETURN_ERROR(stream, "invalid wire_type");
     }
 }
 
@@ -197,7 +197,7 @@ bool checkreturn pb_make_string_substream(pb_istream_t *stream, pb_istream_t *su
     
     *substream = *stream;
     if (substream->bytes_left < size)
-        return false;
+        PB_RETURN_ERROR(stream, "parent stream too short");
     
     substream->bytes_left = size;
     stream->bytes_left -= size;
@@ -316,7 +316,7 @@ static bool checkreturn decode_field(pb_istream_t *stream, pb_wire_type_t wire_t
                 size_t *size = (size_t*)iter->pSize;
                 void *pItem = (uint8_t*)iter->pData + iter->current->data_size * (*size);
                 if (*size >= iter->current->array_size)
-                    return false;
+                    PB_RETURN_ERROR(stream, "array overflow");
                 
                 (*size)++;
                 return func(stream, iter->current, pItem);
@@ -339,7 +339,7 @@ static bool checkreturn decode_field(pb_istream_t *stream, pb_wire_type_t wire_t
                 while (substream.bytes_left)
                 {
                     if (!pCallback->funcs.decode(&substream, iter->current, pCallback->arg))
-                        return false;
+                        PB_RETURN_ERROR(stream, "callback failed");
                 }
                 
                 pb_close_string_substream(stream, &substream);
@@ -364,7 +364,7 @@ static bool checkreturn decode_field(pb_istream_t *stream, pb_wire_type_t wire_t
         }
         
         default:
-            return false;
+            PB_RETURN_ERROR(stream, "invalid field type");
     }
 }
 
@@ -463,7 +463,7 @@ bool checkreturn pb_decode(pb_istream_t *stream, const pb_field_t fields[], void
             iter.required_field_index < PB_MAX_REQUIRED_FIELDS &&
             !(fields_seen[iter.required_field_index >> 3] & (1 << (iter.required_field_index & 7))))
         {
-            return false;
+            PB_RETURN_ERROR(stream, "missing required field");
         }
     } while (pb_field_next(&iter));
     
@@ -539,7 +539,7 @@ bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_t *field, vo
         case 2: *(uint16_t*)dest = value; break;
         case 4: *(uint32_t*)dest = value; break;
         case 8: *(uint64_t*)dest = value; break;
-        default: return false;
+        default: PB_RETURN_ERROR(stream, "invalid data_size");
     }
     
     return status;
@@ -554,7 +554,7 @@ bool checkreturn pb_dec_svarint(pb_istream_t *stream, const pb_field_t *field, v
     {
         case 4: *(int32_t*)dest = value; break;
         case 8: *(int64_t*)dest = value; break;
-        default: return false;
+        default: PB_RETURN_ERROR(stream, "invalid data_size");
     }
     
     return status;
@@ -583,7 +583,7 @@ bool checkreturn pb_dec_bytes(pb_istream_t *stream, const pb_field_t *field, voi
     
     /* Check length, noting the space taken by the size_t header. */
     if (x->size > field->data_size - offsetof(pb_bytes_array_t, bytes))
-        return false;
+        PB_RETURN_ERROR(stream, "bytes overflow");
     
     return pb_read(stream, x->bytes, x->size);
 }
@@ -597,7 +597,7 @@ bool checkreturn pb_dec_string(pb_istream_t *stream, const pb_field_t *field, vo
     
     /* Check length, noting the null terminator */
     if (size + 1 > field->data_size)
-        return false;
+        PB_RETURN_ERROR(stream, "string overflow");
     
     status = pb_read(stream, (uint8_t*)dest, size);
     *((uint8_t*)dest + size) = 0;
@@ -613,7 +613,7 @@ bool checkreturn pb_dec_submessage(pb_istream_t *stream, const pb_field_t *field
         return false;
     
     if (field->ptr == NULL)
-        return false;
+        PB_RETURN_ERROR(stream, "invalid field descriptor");
     
     status = pb_decode(&substream, (pb_field_t*)field->ptr, dest);
     pb_close_string_substream(stream, &substream);
diff --git a/pb_decode.h b/pb_decode.h
index 2880c070..ad45efbf 100644
--- a/pb_decode.h
+++ b/pb_decode.h
@@ -33,6 +33,10 @@ struct _pb_istream_t
     bool (*callback)(pb_istream_t *stream, uint8_t *buf, size_t count);
     void *state; /* Free field for use by callback implementation */
     size_t bytes_left;
+    
+#ifndef PB_NO_ERRMSG
+    const char *errmsg;
+#endif
 };
 
 pb_istream_t pb_istream_from_buffer(uint8_t *buf, size_t bufsize);
diff --git a/tests/test_decode1.c b/tests/test_decode1.c
index 78781dd4..64eb4689 100644
--- a/tests/test_decode1.c
+++ b/tests/test_decode1.c
@@ -69,7 +69,7 @@ int main()
     /* Decode and print out the stuff */
     if (!print_person(&stream))
     {
-        printf("Parsing failed.\n");
+        printf("Parsing failed: %s\n", PB_GET_ERROR(&stream));
         return 1;
     } else {
         return 0;
diff --git a/tests/test_decode2.c b/tests/test_decode2.c
index d38e6256..762b2b3f 100644
--- a/tests/test_decode2.c
+++ b/tests/test_decode2.c
@@ -82,7 +82,7 @@ int main()
     pb_istream_t stream = {&callback, stdin, 10000};
     if (!print_person(&stream))
     {
-        printf("Parsing failed.\n");
+        printf("Parsing failed: %s\n", PB_GET_ERROR(&stream));
         return 1;
     } else {
         return 0;
diff --git a/tests/test_decode3.c b/tests/test_decode3.c
index 30a8afa7..4f55b558 100644
--- a/tests/test_decode3.c
+++ b/tests/test_decode3.c
@@ -89,7 +89,7 @@ int main()
     /* Decode and print out the stuff */
     if (!check_alltypes(&stream))
     {
-        printf("Parsing failed.\n");
+        printf("Parsing failed: %s\n", PB_GET_ERROR(&stream));
         return 1;
     } else {
         return 0;
diff --git a/tests/test_missing_fields.c b/tests/test_missing_fields.c
index 46cd7d96..27741847 100644
--- a/tests/test_missing_fields.c
+++ b/tests/test_missing_fields.c
@@ -27,7 +27,7 @@ int main()
         
         if (!pb_decode(&stream, MissingField_fields, &msg))
         {
-            printf("Decode failed.\n");
+            printf("Decode failed: %s\n", PB_GET_ERROR(&stream));
             return 2;
         }
     }