Re: [PATCH 1/4] mdadm/util:integrate stat operations into one utility

[Zhilong Liu <zlliu@suse.com>]

On 04/17/2017 03:08 PM, Liu Zhilong wrote:
>
>
> On 04/14/2017 11:20 PM, Jes Sorensen wrote:
>> On 04/14/2017 06:14 AM, Liu Zhilong wrote:
>>>
>>>
>>> On 04/05/2017 11:42 PM, jes.sorensen@gmail.com wrote:
>>>> Zhilong Liu <zlliu@suse.com> writes:
>> [snip]
>>>> For a function like this, lets name it better md_is_blkdev()
>>>
>>> How about return the devid after checking? Because always need the
>>> stb.st_rdev to parse
>>> the major and minor number. Although "util.c" has "devnm2devid" to
>>> gather the devid via
>>> to devnm, but it's convenient to return devid when check the blkdev 
>>> with
>>> absolute path.
>>> would you mind the function like this?
>>>
>>> // returns dev-id when success, return 0 when failure
>>> dev_t stat_md_is_blkdev(char *dev)
>>> {
>>>     struct stat stb;
>>>
>>>     if (stat(dev, &stb) != 0) {
>>>         pr_err("stat failed for %s: %s\n", dev, strerror(errno));
>>>         return 0;
>>>     }
>>>     if ((S_IFMT & stb.st_mode) != S_IFBLK) {
>>>         pr_err("%s is not a block device.\n", dev);
>>>         return 0;
>>>     }
>>>     return stb.st_rdev;
>>> }
>>
>> I am generally wary of too many smart handlers, but I think it makes 
>> some sense here to de-duplicate the repeated code.
>>
>> That said, your function would ditch the error information, and the 
>> caller wouldn't know why it failed. If you made it more like this and 
>> return the error code, plus stick the major/minor number into rdev, 
>> if one is provided:
>>
>> int stat_md_is_blkdev(char *devname, *dev_t rdev)
>> {
>> }
>>
>
> I want to integrate two situations into one function.
> 1. only check the 'dev'(such as /dev/loop2) whether or not is block 
> device.
> 2. optionally return the rdev-id after checking the block device.
> this sample "int stat_md_is_blkdev(char *devname, *dev_t rdev)" is 
> smart for the situation 2. but for situation 1, the second parameter 
> is a little waste.
>
> how about like this?
> dev_t stat_md_is_blkdev(char *devname, bool require_rdev)
> {
>     struct stat stb;
>
>        if (stat(devname, &stb) != 0) {
>                printf("stat failed for %s.\n", devname);

in addition:  the errno wouldn't be missed in the final patch.
pr_err("stat failed for %s: %s\n", dev, strerror(errno));

Thanks,
-Zhilong
>                return 1;
>        }
>        if ((S_IFMT & stb.st_mode) != S_IFBLK) {
>                printf("%s is not a block device.\n", devname);
>                return 1;
>        }
>        if (require_rdev)
>           return stb.st_rdev;
>     return 0;
> }
>
> the caller would be like this,
> situation 1:
> char dev[20] = "/dev/loop2";
> if (stat_md_is_blkdev(dev, false))
>   return 1;
>
> situation 2:
> dev_t st_rdev;
> char dev[20] = "/dev/loop2";
> st_rdev = stat_md_is_blkdev(dev, true);
> if (st_rdev == 1)
>   return 1;
>
>
> Thanks very much,
> -Zhilong
>
>> Jes
>>
>>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>