diff mbox

Fix error introduced in a15f05f1b43d8e85d9a3f72a0a

Message ID 1453917321-31242-1-git-send-email-suraev@alumni.ntnu.no
State Changes Requested
Headers show

Commit Message

Suraev Jan. 27, 2016, 5:55 p.m. UTC 
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(-)

Comments

Holger Freyther Jan. 27, 2016, 7:49 p.m. UTC | #1 
> 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?
Suraev Jan. 28, 2016, 9:04 a.m. UTC | #2 
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.
diff mbox

Patch

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