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

[Jes Sorensen <jes.sorensen@gmail.com>]

On 04/17/2017 03:08 AM, 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);
>                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;
>

No, this is really ugly - if you have a function that exhibits two 
different behaviors based on a boolean flag, you should have two 
versions. Parse a pointer for an optional return of dev_t and return the 
error value.

Jes