linux-kernel - Re: [PATCH 1/5] ocxl: Rename struct link to ocxl

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]

Message-Id: <13209edc-d027-4d2f-668b-ad969a25eb06@linux.ibm.com>
Date:   Wed, 27 Feb 2019 14:45:44 +0100
From:   Frederic Barrat <fbarrat@...ux.ibm.com>
To:     Andrew Donnellan <andrew.donnellan@....ibm.com>,
        "Alastair D'Silva" <alastair@...ilva.org>,
        "'Alastair D'Silva'" <alastair@....ibm.com>
Cc:     "'Greg Kurz'" <groug@...d.org>, "'Arnd Bergmann'" <arnd@...db.de>,
        "'Greg Kroah-Hartman'" <gregkh@...uxfoundation.org>,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] ocxl: Rename struct link to ocxl_link



Le 27/02/2019 à 09:18, Andrew Donnellan a écrit :
> On 27/2/19 7:04 pm, Alastair D'Silva wrote:
>>> -----Original Message-----
>>> From: Andrew Donnellan <andrew.donnellan@....ibm.com>
>>> Sent: Wednesday, 27 February 2019 6:55 PM
>>> To: Alastair D'Silva <alastair@...ilva.org>; 'Alastair D'Silva'
>>> <alastair@....ibm.com>
>>> Cc: 'Greg Kurz' <groug@...d.org>; 'Frederic Barrat'
>>> <fbarrat@...ux.ibm.com>; 'Arnd Bergmann' <arnd@...db.de>; 'Greg Kroah-
>>> Hartman' <gregkh@...uxfoundation.org>; linuxppc-dev@...ts.ozlabs.org;
>>> linux-kernel@...r.kernel.org
>>> Subject: Re: [PATCH 1/5] ocxl: Rename struct link to ocxl_link
>>>
>>> On 27/2/19 6:34 pm, Alastair D'Silva wrote:>>> diff --git
>>> a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index
>>>>>> e6a607488f8a..16eb8a60d5c7 100644
>>>>>> --- a/drivers/misc/ocxl/file.c
>>>>>> +++ b/drivers/misc/ocxl/file.c
>>>>>> @@ -152,7 +152,7 @@ static long afu_ioctl_enable_p9_wait(struct
>>>>>> ocxl_context *ctx,
>>>>>>
>>>>>>             if (status == ATTACHED) {
>>>>>>                 int rc;
>>>>>> -            struct link *link = ctx->afu->fn->link;
>>>>>> +            void *link = ctx->afu->fn->link;
>>>>>
>>>>> This doesn't look like a rename...
>>>>
>>>> That corrects the type to what the member (and prototype for
>>> ocxl_link_update_pe) declare it as.
>>>>
>>>> The struct link there is bogus, it shouldn't even compile (since the 
>>>> intended
>>> struct link is defined in a different compilation unit), but instead 
>>> picks up a
>>> different definition of 'struct link' from elsewhere.
>>>>
>>>
>>> Given there's only a handful of struct links defined across the 
>>> entire kernel,
>>> I'm going to guess that the definition it's picking up is in fact the 
>>> ocxl one.
>>>
>>
>> Unlikely, since that's never in a header. It wasn't caught since it 
>> was assigned to/from a void*.
> 
> Ah, yeah that'd explain it... and it's a pointer so it never needs to 
> know its size. I'm clearly not very good at C.
> 
>>
>>> I think the better solution here is to move struct ocxl_link into
>>> ocxl_internal.h, change ocxl_fn::link to be struct ocxl_link * rather 
>>> than void
>>> *, and update the function signature for ocxl_link_update_pe() as well.
>> Not move it, but we could have an opaque declaration there.
>>
> 
> Putting it there would fit with all the other ocxl_* structs, but either 
> way, we definitely need a declaration in there and get rid of the void*, t


Mmm, it might turn out to be more invasive that planned...
The point was only to have it as an opaque to the outside world, for 
APIs we'd like to deprecate at some point, so I wouldn't sweat too much 
over it.

   Fred