From: Max msuraev@sysmocom.de
--- src/bitvec.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/bitvec.c b/src/bitvec.c index f9341b7..7254674 100644 --- a/src/bitvec.c +++ b/src/bitvec.c @@ -383,14 +383,12 @@ unsigned int bitvec_unpack(struct bitvec *bv, const uint8_t *buffer)
int bitvec_unhex(struct bitvec *bv, const char *src) { - unsigned val; - unsigned write_index = 0; - unsigned digits = bv->data_len * 2; - for (unsigned i = 0; i < digits; i++) { - if (sscanf(src + i, "%1x", &val) < 1) { + unsigned val, i, write_index = 0; + for (i = 0; i < bv->data_len * 2; i++) { + if (sscanf(src + i, "%1x", &val) < 1) return 1; - } - bitvec_write_field(bv, write_index,val, 4); + + bitvec_write_field(bv, write_index, val, 4); } return 0; }
From: Max msuraev@sysmocom.de
The code in bitvec_*_field was incorrectly ported from C++. This also affected bitvec_unhex which uses it. --- src/bitvec.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/bitvec.c b/src/bitvec.c index 7254674..407dfc8 100644 --- a/src/bitvec.c +++ b/src/bitvec.c @@ -389,6 +389,7 @@ int bitvec_unhex(struct bitvec *bv, const char *src) return 1;
bitvec_write_field(bv, write_index, val, 4); + write_index += 4; } return 0; } @@ -407,7 +408,7 @@ uint64_t bitvec_read_field(struct bitvec *bv, unsigned int read_index, unsigned ui |= ((uint64_t)1 << (len - i - 1)); bv->cur_bit++; } - read_index += len; + return ui; }
@@ -425,8 +426,8 @@ int bitvec_write_field(struct bitvec *bv, unsigned int write_index, uint64_t val if (rc) return rc; } - write_index += len; - return 0; + + return write_index + len; }
/*! @} */
On 28 Jan 2016, at 12:28, suraev@alumni.ntnu.no wrote:
Dear Max,
The code in bitvec_*_field was incorrectly ported from C++.
and this is why a good commit message is so important. I assumed you had simply copied the code from the PCU to libosmocore. I had no indication that you had to change the semantic as part of C++ -> C. It would have been important to write the thoughts of how you dealt with the C++ reference and converted it to C.
After looking at the PCU I think the change from reference to pass by value is a dangerous decision.
bitvec_write_field(vector, writeIndex, *pui8, no_of_bits);
The code will silently continue to compile and this is why I think the signature of bitvec_write_field and bitvec_read_field needs to be a pointer to not have a huge mess with the osmo_pcu code.
holger
From: Max msuraev@sysmocom.de
--- src/bitvec.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/bitvec.c b/src/bitvec.c index 407dfc8..2221142 100644 --- a/src/bitvec.c +++ b/src/bitvec.c @@ -138,6 +138,7 @@ unsigned int bitvec_get_nth_set_bit(const struct bitvec *bv, unsigned int n) * \param[in] bv bit vector on which to operate * \param[in] bitnr number of bit to be set * \param[in] bit value to which the bit is to be set + * \returns 0 on success, negative value on error */ int bitvec_set_bit_pos(struct bitvec *bv, unsigned int bitnr, enum bit_value bit) @@ -163,6 +164,7 @@ int bitvec_set_bit_pos(struct bitvec *bv, unsigned int bitnr, /*! \brief set the next bit inside a bitvec * \param[in] bv bit vector to be used * \param[in] bit value of the bit to be set + * \returns 0 on success, negative value on error */ int bitvec_set_bit(struct bitvec *bv, enum bit_value bit) { @@ -394,6 +396,12 @@ int bitvec_unhex(struct bitvec *bv, const char *src) return 0; }
+/*! \brief read part of the vector + * \param[in] bv The boolean vector to work on + * \param[in] read_index Where reading supposed to start in the vector + * \param[in] len How many bits to read from vector + * \returns read bits or negative value on error + */ uint64_t bitvec_read_field(struct bitvec *bv, unsigned int read_index, unsigned int len) { unsigned int i; @@ -412,7 +420,12 @@ uint64_t bitvec_read_field(struct bitvec *bv, unsigned int read_index, unsigned return ui; }
- +/*! \brief write into the vector + * \param[in] bv The boolean vector to work on + * \param[in] write_index Where writing supposed to start in the vector + * \param[in] len How many bits to write + * \returns next write index or negative value on error + */ int bitvec_write_field(struct bitvec *bv, unsigned int write_index, uint64_t val, unsigned int len) { unsigned int i;
From: Max msuraev@sysmocom.de
--- tests/bitvec/bitvec_test.c | 16 ++++++++++++++++ tests/bitvec/bitvec_test.ok | 15 +++++++++++++++ 2 files changed, 31 insertions(+)
diff --git a/tests/bitvec/bitvec_test.c b/tests/bitvec/bitvec_test.c index 624e334..789df75 100644 --- a/tests/bitvec/bitvec_test.c +++ b/tests/bitvec/bitvec_test.c @@ -55,8 +55,24 @@ static void test_byte_ops() printf("=== end %s ===\n", __func__); }
+static void test_unhex(const char *hex) +{ + struct bitvec b; + uint8_t d[64] = {0}; + b.data = d; + b.data_len = sizeof(d); + b.cur_bit = 0; + printf("%d -=>\n", bitvec_unhex(&b, hex)); + printf("%s\n%s\n", osmo_hexdump_nospc(d, 64), osmo_hexdump_nospc((const unsigned char *)hex, 23)); +} + int main(int argc, char **argv) { test_byte_ops(); + test_unhex("48282407a6a074227201000b2b2b2b2b2b2b2b2b2b2b2b"); + test_unhex("47240c00400000000000000079eb2ac9402b2b2b2b2b2b"); + test_unhex("47283c367513ba333004242b2b2b2b2b2b2b2b2b2b2b2b"); + test_unhex("DEADFACE000000000000000000000000000000BEEFFEED"); + test_unhex("FFFFFAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"); return 0; } diff --git a/tests/bitvec/bitvec_test.ok b/tests/bitvec/bitvec_test.ok index 1f329af..8d944bc 100644 --- a/tests/bitvec/bitvec_test.ok +++ b/tests/bitvec/bitvec_test.ok @@ -1,2 +1,17 @@ === start test_byte_ops === === end test_byte_ops === +1 -=> +48282407a6a074227201000b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +48282407a6a074227201000b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +1 -=> +47240c00400000000000000079eb2ac9402b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +47240c00400000000000000079eb2ac9402b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +1 -=> +47283c367513ba333004242b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +47283c367513ba333004242b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +1 -=> +deadface000000000000000000000000000000beeffeed0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +deadface000000000000000000000000000000beeffeed0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +0 -=> +fffffaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +fffffaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
On 28 Jan 2016, at 12:28, suraev@alumni.ntnu.no wrote:
From: Max msuraev@sysmocom.de
Dear Max,
one can put fix and testcase into the same commit, if you decide to split it up then first commit the testcase with an expected failure and then when you fix things, fix the test result. This will show/track the change in behavior.
In my initial review I had asked for a stronger post condition and I still think this applies to the test. I see we have one truncation test but I don't see a test that checks how many bits have been taken from the hexstring.
tests/bitvec/bitvec_test.c | 16 ++++++++++++++++ tests/bitvec/bitvec_test.ok | 15 +++++++++++++++ 2 files changed, 31 insertions(+)
diff --git a/tests/bitvec/bitvec_test.c b/tests/bitvec/bitvec_test.c index 624e334..789df75 100644 --- a/tests/bitvec/bitvec_test.c +++ b/tests/bitvec/bitvec_test.c @@ -55,8 +55,24 @@ static void test_byte_ops() printf("=== end %s ===\n", __func__); }
+static void test_unhex(const char *hex) +{
- struct bitvec b;
- uint8_t d[64] = {0};
- b.data = d;
- b.data_len = sizeof(d);
- b.cur_bit = 0;
- printf("%d -=>\n", bitvec_unhex(&b, hex));
- printf("%s\n%s\n", osmo_hexdump_nospc(d, 64), osmo_hexdump_nospc((const unsigned char *)hex, 23));
+}
int main(int argc, char **argv) { test_byte_ops();
- test_unhex("48282407a6a074227201000b2b2b2b2b2b2b2b2b2b2b2b");
- test_unhex("47240c00400000000000000079eb2ac9402b2b2b2b2b2b");
- test_unhex("47283c367513ba333004242b2b2b2b2b2b2b2b2b2b2b2b");
- test_unhex("DEADFACE000000000000000000000000000000BEEFFEED");
- test_unhex("FFFFFAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"); return 0;
} diff --git a/tests/bitvec/bitvec_test.ok b/tests/bitvec/bitvec_test.ok index 1f329af..8d944bc 100644 --- a/tests/bitvec/bitvec_test.ok +++ b/tests/bitvec/bitvec_test.ok @@ -1,2 +1,17 @@ === start test_byte_ops === === end test_byte_ops === +1 -=> +48282407a6a074227201000b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +48282407a6a074227201000b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +1 -=> +47240c00400000000000000079eb2ac9402b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +47240c00400000000000000079eb2ac9402b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +1 -=> +47283c367513ba333004242b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +47283c367513ba333004242b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +1 -=> +deadface000000000000000000000000000000beeffeed0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +deadface000000000000000000000000000000beeffeed0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +0 -=> +fffffaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+fffffaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
2.5.0
-
Holger Freyther -
suraev＠alumni.ntnu.no