Re: [PATCH v3 2/2] mdadm: Introduce new array state 'broken' for raid0/linear

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux