Osa enhancements#378
Conversation
allowed 1 hex char (max 0xf) but needs 2 for the 5-bit Gen6 mask (0x1f) allowed 2 hex chars (max 0xff) but needs 3 for the 9-bit mask (0x1ff)
Fix alignment issue, previously Gen5 ended with newline so Gen6 was forced to new line, removed so all gens remain on one line. Pattern section was using the os_type link rate instead of the pattern link rate. Fixed logical AND which should be bitwise.
| if (cfg.os_types) { | ||
| ret = convert_hex_str(cfg.os_types, &os_type_mask, | ||
| &num_dwords, 1); | ||
| &num_dwords, 2); |
There was a problem hiding this comment.
I'm finding this code extremely difficult to understand. convert_hex_str() is weird. The documentation for os_types seems to indicate it just inputs a hex string like "0xA" with only 4 valid bits which should be really simple and shouldn't need a bunch of memory allocations and arrays of integers.
CFG_LONG already supports strings that have a 0x prefix so they can be specified in hexadecimal without all this extra work.
The documentation above for the command line parameter says there are only 4 bits in os_types. But this seems to be increasing it to take two hex characters (I assume). Then num_dwords is confusing because a d-word usually refers to something that is 4-bytes long or 8 hex characters.
There was a problem hiding this comment.
Hi Logan, originally I used the convert_hex_str on a string of multiple dword hexadecimal values hence the allocating of arrays and such. But later down the line I also applied it to be used with regular hex values that weren't even dwords, without much thought as to easier solutions (as you mentioned). So I refactor the OSA functions which use convert_hex_str instead to use the default CFG_ args that have built-in 0x prefix handling. Some functions still use convert_hex_str but those are the original functions which needed it for multiple dwords in a string.
Hope that clears it up a little.
Thanks
convert_hex_str was being used too much throughout the code, allocated/ freeing memory unnecessarily. Replace hex string argument with default int argument.
lsgunth
left a comment
lsgunth
left a comment
There was a problem hiding this comment.
That makes more sense, thanks!
2 participants
No description provided.