linux-kernel - Re: [RFCv4 01/21] media: add request API core and UAPI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 22 Feb 2018 10:38:17 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Alexandre Courbot <acourbot@...omium.org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Pawel Osciak <posciak@...omium.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Tomasz Figa <tfiga@...omium.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Gustavo Padovan <gustavo.padovan@...labora.com>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFCv4 01/21] media: add request API core and UAPI

On 02/22/18 10:30, Alexandre Courbot wrote:
> On Wed, Feb 21, 2018 at 4:29 PM, Hans Verkuil <hverkuil@...all.nl> wrote:
>> On 02/21/2018 07:01 AM, Alexandre Courbot wrote:
>>> Hi Hans,
>>>
>>> On Tue, Feb 20, 2018 at 7:36 PM, Hans Verkuil <hverkuil@...all.nl> wrote:
>>>> On 02/20/18 05:44, Alexandre Courbot wrote:
>>
>> <snip>
>>
>>>>> +#define MEDIA_REQUEST_IOC(__cmd, func)                                       \
>>>>> +     [_IOC_NR(MEDIA_REQUEST_IOC_##__cmd) - 0x80] = {                 \
>>>>> +             .cmd = MEDIA_REQUEST_IOC_##__cmd,                       \
>>>>> +             .fn = func,                                             \
>>>>> +     }
>>>>> +
>>>>> +struct media_request_ioctl_info {
>>>>> +     unsigned int cmd;
>>>>> +     long (*fn)(struct media_request *req);
>>>>> +};
>>>>> +
>>>>> +static const struct media_request_ioctl_info ioctl_info[] = {
>>>>> +     MEDIA_REQUEST_IOC(SUBMIT, media_request_ioctl_submit),
>>>>> +     MEDIA_REQUEST_IOC(REINIT, media_request_ioctl_reinit),
>>>>
>>>> There are only two ioctls, so there is really no need for the
>>>> MEDIA_REQUEST_IOC define. Just keep it simple.
>>>
>>> The number of times it is used doesn't change the fact that it helps
>>> with readability IMHO.
>>
>> But this macro just boils down to:
>>
>> static const struct media_request_ioctl_info ioctl_info[] = {
>>         { MEDIA_REQUEST_IOC_SUBMIT, media_request_ioctl_submit },
>>         { MEDIA_REQUEST_IOC_REINIT, media_request_ioctl_reinit },
>> };
>>
>> It's absolutely identical! So it seems senseless to me.
> 
> This expands to more than that - the index needs to be offset by 0x80,
> something we probably don't want to repeat every line.
> 
>>
>>>
>>>>
>>>>> +};
>>>>> +
>>>>> +static long media_request_ioctl(struct file *filp, unsigned int cmd,
>>>>> +                             unsigned long __arg)
>>>>> +{
>>>>> +     struct media_request *req = filp->private_data;
>>>>> +     const struct media_request_ioctl_info *info;
>>>>> +
>>>>> +     if ((_IOC_NR(cmd) < 0x80) ||
>>>>
>>>> Why start the ioctl number at 0x80? Why not just 0?
>>>> It avoids all this hassle with the 0x80 offset.
>>
>> There is no clash with the MC ioctls, so I really don't believe the 0x80
>> offset is needed.
> 
> I suppose your comment in patch 16 supersedes this one. :)

Yes, it does. I realized later why this was done like this.

That said, I don't like the magic value. Something like this might be
cleaner:

	const unsigned int first_ioc_nr = _IOC_NR(MEDIA_REQUEST_IOC_SUBMIT);

Then use first_ioc_nr (or nr_offset or whatever) instead of 0x80.

Regards,

	Hans