mbus_parse_hex -n ignores parsing problem

[fredrik-sk]

PR #168 executes the code for normalized output, which probably has not been executed so much until now.

As found here https://github.com/rscada/libmbus/pull/168#issuecomment-651723887 running mbus_hex_parse -n sen_pollutherm.hex causes some errors to be printed but the program returns with exitcode 0.

Adding an else statment like this

--- a/mbus/mbus-protocol-aux.c
+++ b/mbus/mbus-protocol-aux.c
@@ -1455,6 +1455,8 @@ mbus_data_variable_xml_normalized(mbus_data_variable *data)
             }
             else
             {
+                free(buff);
+                return NULL;
             }

             len += snprintf(&buff[len], buff_size - len, "    </DataRecord>\n\n");

makes the test output this:

mbus_vif_unit_normalize: Unknown VIF 0x07B
mbus_vib_unit_normalize: Error mbus_vif_unit_normalize
mbus_parse_variable_record: problem with mbus_vib_unit_normalize
Failed to generate XML representation of MBUS frame:
Unable to generate XML for test/test-frames/sen_pollutherm.hex

[fredrik-sk]

Can someone that has access to the specification check if VIF=0x7B is allowed?

According to https://m-bus.com/documentation-wired/08-appendix, I cannot se anything that indicates it. Or are we off when decoding the data?

sen_pollutherm.hex after some re-formating

68 42 42 68 08 08 
72 76 00 05 21 18 4E 31 04 51 00 00 00
0C 07 64 08 00 00
0C 14 92 98 79 00 
0C 7B 02 03 00 00  # <==
0C 2C 58 54 00 00 
0A 5A 55 07 0A 5E 94 05 0B 60 76 60 01 0C 78 76 00 05 21 0C FD 10 76 00 05 21 1F B3 16

[lategoodbye]

I prefer to fix the decoding issue instead of disabling the complete XML output.

[fredrik-sk]

I prefer to fix the decoding issue instead of disabling the complete XML output.

@lategoodbye : Yes, fixing a decoding error is something we ALSO should do, but according to me, that is a separate issue.

The suggested update in #170, 7297899a89d727fcf1894f4e0253f99d94bca9cb, follows the rule "fail early, fail hard" to make sure that problems are noticed and not ignored.

This change will make a whole class of potential problems detectable when running tests.

[lategoodbye]

I've a different opinion about this. There will always a byte the libmbus cannot decode and the public available document won't save us everytime. From a unit test point of view the suggested "fail early, fail hard" is good, but from end user perspective i'm concerned that this could be seen as a regression and the users tend to use older versions.

So i prefer to keep the best effort XML output in error case, but make mbus_parse_hex fail with an error code.