Re: [PATCH 3/3] virtio_ring: Use const to annotate read-only pointer params

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Feng Liu <feliu@nvidia.com>
Cc: "virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>,
	"feng.liu.kernel@gmail.com" <feng.liu.kernel@gmail.com>,
	Jiri Pirko <jiri@nvidia.com>, Bodong Wang <bodong@nvidia.com>,
	Gavin Li <gavinl@nvidia.com>
Subject: Re: [PATCH 3/3] virtio_ring: Use const to annotate read-only pointer params
Date: Wed, 8 Mar 2023 11:49:51 -0500	[thread overview]
Message-ID: <20230308114919-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20230308110235-mutt-send-email-mst@kernel.org>

On Wed, Mar 08, 2023 at 11:26:04AM -0500, Michael S. Tsirkin wrote:
> On Wed, Mar 08, 2023 at 10:59:57AM -0500, Feng Liu wrote:
> > 
> > 
> > On 2023-03-08 a.m.9:13, Michael S. Tsirkin wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote:
> > > > On 2023-03-07 04:14, David Edmondson wrote:
> > > > > External email: Use caution opening links or attachments
> > > > > 
> > > > > 
> > > > > Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
> > > > > writes:
> > > > > 
> > > > > > Add const to make the read-only pointer parameters clear, similar to
> > > > > > many existing functions.
> > > > > 
> > > > > In many of the modified functions the local variable that is a cast of
> > > > > the argument could also be const. Is there a reason not to do both at
> > > > > the same time?
> > > > > 
> > > > 
> > > > Hi,David
> > > > 
> > > > In order to prevent the content of a pointer parameter from being
> > > > modified and increase the readability of the function, it is recommended
> > > > to add the 'const' keyword to the parameter. This is not necessary for
> > > > local variables and non-pointer parameters, as they are only stored on
> > > > the stack and do not affect the original value or structure member
> > > > passed into the function. Therefore, in this case, the 'const' keyword
> > > > is only added to pointer parameters.
> > > 
> > > This makes no sense to me. If ytou cast away the const then it is
> > > pointless.
> > > 
> > 
> > Hi, Michael
> > 
> > I really don't quite understand your point of view.
> > Is a local variable that needs to be add const? Can you help to point
> > out the specific problem/point ?
> 
> I just repeated what David said.  Basically most of these functions use
> to_vvq which uses container_of which in turn loses const qualifier.
> So your change is poinless since rest of code accesses vq through
> to_vvq.
> 
> What to do? I don't like the idea of to_vvq_const.
> So I propose a version of container_of using _Generic
> which preserves the const qualifier.
> 
> 
> #define container_of(ptr, type, member) \
>     ({ \
>         const void *__mptr = (ptr); \
>         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
>                       __same_type(*(ptr), void),                        \
>                       "pointer type mismatch in container_of()");       \
>         _Generic((ptr), \
>                  typeof(&((const type *)0)->member): \
>                  (const type *)(__mptr - offsetof(type, member)), \
>                  default: \
>                  (type *)(__mptr - offsetof(type, member))); \
>     })
> 
> 
> I'll hack it up in a day or two and post.
> 

Oh wait a second. There's already container_of_const.
So just use it in to_vvq.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization