Review at https://gerrit.osmocom.org/5535
cosmetic: clarify coding scheme and puncturing
* use appropriate types for coding scheme parameters
* add comment regarding possible number of RLCMAC blocks
The code in create_dl_acked_block() has underlying assumption that
rlc.num_data_blocks can never be more than 2, which is true and is
enforced by appropriate asserts but is not obvious when looking at the
function code alone. It's equally hard for Coverity which leads to false
positives in scan.
Lets' make this assumption explicit by putting it into for(;;) condition
alongside with corresponding comment.
Fixes: CID143070
Change-Id: If599a6c8a6ef56d847604fcf41bb71decccd8a78
---
M src/gprs_coding_scheme.cpp
M src/gprs_coding_scheme.h
M src/rlc.cpp
M src/rlc.h
M src/tbf_dl.cpp
5 files changed, 45 insertions(+), 45 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/35/5535/1
diff --git a/src/gprs_coding_scheme.cpp b/src/gprs_coding_scheme.cpp
index 719cbb2..3094ae6 100644
--- a/src/gprs_coding_scheme.cpp
+++ b/src/gprs_coding_scheme.cpp
@@ -55,12 +55,12 @@
static struct {
struct {
- unsigned int bytes;
- unsigned int ext_bits;
- unsigned int data_header_bits;
+ uint8_t bytes;
+ uint8_t ext_bits;
+ uint8_t data_header_bits;
} uplink, downlink;
- unsigned int data_bytes;
- unsigned int optional_padding_bits;
+ uint8_t data_bytes;
+ uint8_t optional_padding_bits;
const char *name;
GprsCodingScheme::HeaderType data_hdr;
GprsCodingScheme::Family family;
@@ -99,10 +99,10 @@
static struct {
struct {
- int data_header_bits;
+ uint8_t data_header_bits;
} uplink, downlink;
- unsigned int data_block_header_bits;
- unsigned int num_blocks;
+ uint8_t data_block_header_bits;
+ uint8_t num_blocks;
const char *name;
} hdr_type_info[GprsCodingScheme::NUM_HEADER_TYPES] = {
{{0}, {0}, 0, 0, "INVALID"},
@@ -134,12 +134,12 @@
return GprsCodingScheme(UNKNOWN);
}
-unsigned int GprsCodingScheme::sizeUL() const
+uint8_t GprsCodingScheme::sizeUL() const
{
return mcs_info[m_scheme].uplink.bytes + (spareBitsUL() ? 1 : 0);
}
-unsigned int GprsCodingScheme::usedSizeUL() const
+uint8_t GprsCodingScheme::usedSizeUL() const
{
if (mcs_info[m_scheme].data_hdr == HEADER_GPRS_DATA)
return mcs_info[m_scheme].uplink.bytes;
@@ -147,22 +147,22 @@
return sizeUL();
}
-unsigned int GprsCodingScheme::maxBytesUL() const
+uint8_t GprsCodingScheme::maxBytesUL() const
{
return mcs_info[m_scheme].uplink.bytes;
}
-unsigned int GprsCodingScheme::spareBitsUL() const
+uint8_t GprsCodingScheme::spareBitsUL() const
{
return mcs_info[m_scheme].uplink.ext_bits;
}
-unsigned int GprsCodingScheme::sizeDL() const
+uint8_t GprsCodingScheme::sizeDL() const
{
return mcs_info[m_scheme].downlink.bytes + (spareBitsDL() ? 1 : 0);
}
-unsigned int GprsCodingScheme::usedSizeDL() const
+uint8_t GprsCodingScheme::usedSizeDL() const
{
if (mcs_info[m_scheme].data_hdr == HEADER_GPRS_DATA)
return mcs_info[m_scheme].downlink.bytes;
@@ -170,42 +170,42 @@
return sizeDL();
}
-unsigned int GprsCodingScheme::maxBytesDL() const
+uint8_t GprsCodingScheme::maxBytesDL() const
{
return mcs_info[m_scheme].downlink.bytes;
}
-unsigned int GprsCodingScheme::spareBitsDL() const
+uint8_t GprsCodingScheme::spareBitsDL() const
{
return mcs_info[m_scheme].downlink.ext_bits;
}
-unsigned int GprsCodingScheme::maxDataBlockBytes() const
+uint8_t GprsCodingScheme::maxDataBlockBytes() const
{
return mcs_info[m_scheme].data_bytes;
}
-unsigned int GprsCodingScheme::optionalPaddingBits() const
+uint8_t GprsCodingScheme::optionalPaddingBits() const
{
return mcs_info[m_scheme].optional_padding_bits;
}
-unsigned int GprsCodingScheme::numDataBlocks() const
+uint8_t GprsCodingScheme::numDataBlocks() const
{
return hdr_type_info[headerTypeData()].num_blocks;
}
-unsigned int GprsCodingScheme::numDataHeaderBitsUL() const
+uint8_t GprsCodingScheme::numDataHeaderBitsUL() const
{
return hdr_type_info[headerTypeData()].uplink.data_header_bits;
}
-unsigned int GprsCodingScheme::numDataHeaderBitsDL() const
+uint8_t GprsCodingScheme::numDataHeaderBitsDL() const
{
return hdr_type_info[headerTypeData()].downlink.data_header_bits;
}
-unsigned int GprsCodingScheme::numDataBlockHeaderBits() const
+uint8_t GprsCodingScheme::numDataBlockHeaderBits() const
{
return hdr_type_info[headerTypeData()].data_block_header_bits;
}
diff --git a/src/gprs_coding_scheme.h b/src/gprs_coding_scheme.h
index dfad240..76cab0f 100644
--- a/src/gprs_coding_scheme.h
+++ b/src/gprs_coding_scheme.h
@@ -70,7 +70,7 @@
operator bool() const {return m_scheme != UNKNOWN;}
operator Scheme() const {return m_scheme;}
- unsigned int to_num() const;
+ uint8_t to_num() const;
GprsCodingScheme& operator =(Scheme s);
bool operator == (Scheme s) const;
@@ -91,20 +91,20 @@
void dec();
void decToSingleBlock(bool *needStuffing);
- unsigned int sizeUL() const;
- unsigned int sizeDL() const;
- unsigned int usedSizeUL() const;
- unsigned int usedSizeDL() const;
- unsigned int maxBytesUL() const;
- unsigned int maxBytesDL() const;
- unsigned int spareBitsUL() const;
- unsigned int spareBitsDL() const;
- unsigned int maxDataBlockBytes() const;
- unsigned int numDataBlocks() const;
- unsigned int numDataHeaderBitsUL() const;
- unsigned int numDataHeaderBitsDL() const;
- unsigned int numDataBlockHeaderBits() const;
- unsigned int optionalPaddingBits() const;
+ uint8_t sizeUL() const;
+ uint8_t sizeDL() const;
+ uint8_t usedSizeUL() const;
+ uint8_t usedSizeDL() const;
+ uint8_t maxBytesUL() const;
+ uint8_t maxBytesDL() const;
+ uint8_t spareBitsUL() const;
+ uint8_t spareBitsDL() const;
+ uint8_t maxDataBlockBytes() const;
+ uint8_t numDataBlocks() const;
+ uint8_t numDataHeaderBitsUL() const;
+ uint8_t numDataHeaderBitsDL() const;
+ uint8_t numDataBlockHeaderBits() const;
+ uint8_t optionalPaddingBits() const;
const char *name() const;
HeaderType headerTypeData() const;
HeaderType headerTypeControl() const;
@@ -127,7 +127,7 @@
enum Scheme m_scheme;
};
-inline unsigned int GprsCodingScheme::to_num() const
+inline uint8_t GprsCodingScheme::to_num() const
{
if (isGprs())
return (m_scheme - CS1) + 1;
diff --git a/src/rlc.cpp b/src/rlc.cpp
index 37e83cd..d7f0609 100644
--- a/src/rlc.cpp
+++ b/src/rlc.cpp
@@ -375,7 +375,7 @@
unsigned int gprs_rlc_mcs_cps(GprsCodingScheme cs,
enum egprs_puncturing_values punct,
- enum egprs_puncturing_values punct2, int with_padding)
+ enum egprs_puncturing_values punct2, bool with_padding)
{
/* validate that punct and punct2 are as expected */
switch (GprsCodingScheme::Scheme(cs)) {
diff --git a/src/rlc.h b/src/rlc.h
index b62e3ac..a7e6cf3 100644
--- a/src/rlc.h
+++ b/src/rlc.h
@@ -163,7 +163,7 @@
unsigned int es_p;
unsigned int rrbp;
unsigned int pr;
- unsigned int num_data_blocks;
+ uint8_t num_data_blocks; /* this can actually be only 0, 1, 2: enforced
in gprs_rlc_data_header_init() */
unsigned int with_padding;
unsigned int data_offs_bits[2];
struct gprs_rlc_data_block_info block_info[2];
@@ -218,7 +218,7 @@
void gprs_rlc_data_block_info_init(struct gprs_rlc_data_block_info *rdbi,
GprsCodingScheme cs, bool with_padding, const unsigned int spb);
unsigned int gprs_rlc_mcs_cps(GprsCodingScheme cs, enum egprs_puncturing_values
- punct, enum egprs_puncturing_values punct2, int with_padding);
+ punct, enum egprs_puncturing_values punct2, bool with_padding);
void gprs_rlc_mcs_cps_decode(unsigned int cps, GprsCodingScheme cs,
int *punct, int *punct2, int *with_padding);
enum egprs_puncturing_values gprs_get_punct_scheme(enum egprs_puncturing_values
diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp
index b043989..3537e85 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -667,7 +667,7 @@
unsigned msg_len;
bool need_poll;
/* TODO: support MCS-7 - MCS-9, where data_block_idx can be 1 */
- unsigned int data_block_idx = 0;
+ uint8_t data_block_idx = 0;
unsigned int rrbp;
uint32_t new_poll_fn;
int rc;
@@ -753,8 +753,8 @@
LOGP(DRLCMACDL, LOGL_DEBUG, "- Copying %u RLC blocks, %u BSNs\n",
rlc.num_data_blocks, num_bsns);
- /* Copy block(s) to RLC message */
- for (data_block_idx = 0; data_block_idx < rlc.num_data_blocks;
+ /* Copy block(s) to RLC message: the num_data_blocks cannot be more
than 2 - see assert above */
+ for (data_block_idx = 0; data_block_idx < OSMO_MIN(rlc.num_data_blocks,
2);
data_block_idx++)
{
int bsn;
@@ -778,7 +778,7 @@
OSMO_ASSERT(m_rlc.block(bsn)->next_ps >= EGPRS_PS_1);
OSMO_ASSERT(m_rlc.block(bsn)->next_ps <= EGPRS_PS_3);
}
- OSMO_ASSERT(data_block_idx < 2); /* punct defined above as
2-element array */
+
punct[data_block_idx] = m_rlc.block(bsn)->next_ps;
rdbi = &rlc.block_info[data_block_idx];
--
To view, visit https://gerrit.osmocom.org/5535
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If599a6c8a6ef56d847604fcf41bb71decccd8a78
Gerrit-PatchSet: 1
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max <msur...@sysmocom.de>
