From: Max msuraev@sysmocom.de
Fix error creeped in while porting code from cpp refs. Add test case for code in question. Expand documentatin to clarify function use. --- src/bitvec.c | 24 +++++++++++++++++++----- tests/bitvec/bitvec_test.c | 15 +++++++++++++++ tests/bitvec/bitvec_test.ok | 12 ++++++++++++ 3 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/src/bitvec.c b/src/bitvec.c index f9341b7..b88afa6 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) { @@ -390,11 +392,18 @@ int bitvec_unhex(struct bitvec *bv, const char *src) 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); + write_index += 4; } 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; @@ -409,11 +418,16 @@ 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; }
- +/*! \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; @@ -427,8 +441,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; }
/*! @} */ diff --git a/tests/bitvec/bitvec_test.c b/tests/bitvec/bitvec_test.c index 624e334..dde8448 100644 --- a/tests/bitvec/bitvec_test.c +++ b/tests/bitvec/bitvec_test.c @@ -55,8 +55,23 @@ static void test_byte_ops() printf("=== end %s ===\n", __func__); }
+static void test_unhex(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(hex, 23)); +} + int main(int argc, char **argv) { test_byte_ops(); + test_unhex("48282407a6a074227201000b2b2b2b2b2b2b2b2b2b2b2b"); + test_unhex("47240c00400000000000000079eb2ac9402b2b2b2b2b2b"); + test_unhex("47283c367513ba333004242b2b2b2b2b2b2b2b2b2b2b2b"); + test_unhex("DEADFACE000000000000000000000000000000BEEFFEED"); return 0; } diff --git a/tests/bitvec/bitvec_test.ok b/tests/bitvec/bitvec_test.ok index 1f329af..17e8fd2 100644 --- a/tests/bitvec/bitvec_test.ok +++ b/tests/bitvec/bitvec_test.ok @@ -1,2 +1,14 @@ === start test_byte_ops === === end test_byte_ops === +1 -=> +48282407a6a074227201000b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +48282407a6a074227201000b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +1 -=> +47240c00400000000000000079eb2ac9402b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +47240c00400000000000000079eb2ac9402b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +1 -=> +47283c367513ba333004242b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +47283c367513ba333004242b2b2b2b2b2b2b2b2b2b2b2b0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +1 -=> +deadface000000000000000000000000000000beeffeed0000000000000000000000000000000000000000000000000000000000000000000000000000000000 +deadface000000000000000000000000000000beeffeed0000000000000000000000000000000000000000000000000000000000000000000000000000000000
On 27 Jan 2016, at 18:55, suraev@alumni.ntnu.no wrote:
From: Max msuraev@sysmocom.de
Fix error creeped in while porting code from cpp refs. Add test case for code in question. Expand documentatin to clarify function use.
Be more specific here. Which routines were impacted? Sure all calls to bitvec_write_field. Why do you decide to not make this variable an in+out variable and instead need to know how many spaces it advanced? Have you checked the other code that was converted?
+static void test_unhex(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(hex, 23));
Extend the test to see what happens if you unhex more than d can hold?
Instead of dumping up to 64bytes can you see how many bytes are filled?
int bitvec_write_field(struct bitvec *bv, unsigned& write_index, uint64_t val, unsigned len) { unsigned int i; int rc; bv->cur_bit = write_index; for (i = 0; i < len; i++) { int bit = 0; if (val & ((uint64_t)1 << (len - i - 1))) bit = 1; rc = bitvec_set_bit(bv, (bit_value)bit); if (rc) return rc; } write_index += len; return 0; }
so if you take the "unsigned&" and make it a plain variable then the write_index += len at the end mkes no sense and can be removed?
27.01.2016 20:49, Holger Freyther пишет:
Be more specific here. Which routines were impacted? Sure all calls to bitvec_write_field.
bitvec_unhex() is the only consumer in libosmocore for bitvec_write_field() so far
Why do you decide to not make this variable an in+out variable and instead need to know how many spaces it advanced?
I've tested that approach as well but it makes code much harder to read without providing any real benefits.
Have you checked the other code that was converted?
Yes, the only potentially problematic functions are bitvec_*_field()
Extend the test to see what happens if you unhex more than d can hold?
Will do.
Instead of dumping up to 64bytes can you see how many bytes are filled?
The functions for that are part of another patch currently under review as well. I did not wanted to reimplement them just for one test-case. I can extend this once it's merged.
so if you take the "unsigned&" and make it a plain variable then the write_index += len at the end mkes no sense and can be removed?
Yes. In the patch in question I instead return write_index+len to make it easier for caller.
cheers, Max.
-
Holger Freyther -
Max (☭) -
suraev＠alumni.ntnu.no