On Mon, Sep 21, 2020 at 06:13:59PM +0000, Rojewski, Cezary wrote:
> On 2020-09-21 2:59 PM, Andy Shevchenko wrote:
> > On Mon, Sep 21, 2020 at 01:54:13PM +0200, Cezary Rojewski wrote:
...
> >> + for (i = j = 0; i < FW_INFO_SIZE_MAX; i++)
> >> + if (cdev->ipc.config.fw_info[i] == ' ')
> >> + if (++j == 4)
> >> + break;
> >
> >> + for (j = ++i; j < FW_INFO_SIZE_MAX && j - i < 20; j++) {
> >
> > This should have static_assert() at the place where you define both constants
> > (2nd is mentioned above 20).
> >
> >> + if (cdev->ipc.config.fw_info[j] == ' ')
> >> + break;
> >> + *(pos + j - i) = cdev->ipc.config.fw_info[j];
> >> + }
> >> + pos += 20;
> >
> > These two for-loops should have some comment to explain what's going on.
> >
>
> Actually, after poking my FW friends again I realized that it's just
> dumping 20chars from "hash" segment of fw_info (struct catpt_fw_ready,
> field: fw_info[]).
>
> So, this could be replaced by:
>
> /* navigate to fifth info segment (fw hash) */
> for (i = j = 0; i < FW_INFO_SIZE_MAX; i++)
> /* info segments are separated by space each */
> if (cdev->ipc.config.fw_info[i] == ' ')
> if (++j == 4)
> break;
...and this is repeating strnchr() / strnchrnul().
With the questions "what if...":
- nul in the middle of this?
- less than 4 spaces found?
> memcpy(pos, &cdev->ipc.config.fw_info[++i], CATPT_DUMP_HASH_SIZE);
> pos += CATPT_DUMP_HASH_SIZE;
>
>
> Existing for-loops were based on internal solution. Half of the code
> isn't needed afterall..
--
With Best Regards,
Andy Shevchenko
