Thanks Neil! CCing Jes, also comments inline:
On 30/08/2019 05:17, NeilBrown wrote:
>> [...]
>> + char arrayst[12] = { 0 }; /* no state is >10 chars currently */
>
> Why do you have an array? Why not just a "char *".
> Then all the "strncpy" below become simple pointer assignment.
OK, makes sense! I'll try to change it.
>> [...]
>> int WaitClean(char *dev, int verbose)
>> {
>> @@ -1116,7 +1120,8 @@ int WaitClean(char *dev, int verbose)
>> rv = read(state_fd, buf, sizeof(buf));
>> if (rv < 0)
>> break;
>> - if (sysfs_match_word(buf, clean_states) <= 4)
>
> Arg. That is horrible. Who wrote that code???
> Oh, it was me. And only 8 years ago.
rofl, happens!
> sysfs_match_word() should return a clear "didn't match" indicator, like
> "-1".
>
> Ideally that should be fixed, but I cannot really expect you to do that.
>
> Maybe make it
> if (clean_states[sysfs_match_word(buf, clean_states)] != NULL)
> ??
> or
> if (sysfs_match_word(buf, clean_states) < ARRAY_SIZE(clean_states)-1)
>
> Otherwise the patch looks ok.
OK, thanks for the review! I'll try to change that in V4 too.
cheers,
Guilherme
