Re: [PATCH vhost v9 3/6] virtio: find_vqs: pass struct instead of multi parameters

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Thu, 20 Jun 2024 07:06:53 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 20, 2024 at 06:43:30PM +0800, Xuan Zhuo wrote:
> > On Thu, 20 Jun 2024 06:15:08 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Thu, Jun 20, 2024 at 05:20:49PM +0800, Xuan Zhuo wrote:
> > > > On Thu, 20 Jun 2024 05:14:24 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > On Thu, Jun 20, 2024 at 05:00:49PM +0800, Xuan Zhuo wrote:
> > > > > > > > @@ -226,21 +248,37 @@ struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev,
> > > > > > > >
> > > > > > > >  static inline
> > > > > > > >  int virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> > > > > > > > -			struct virtqueue *vqs[], vq_callback_t *callbacks[],
> > > > > > > > -			const char * const names[],
> > > > > > > > -			struct irq_affinity *desc)
> > > > > > > > +		    struct virtqueue *vqs[], vq_callback_t *callbacks[],
> > > > > > > > +		    const char * const names[],
> > > > > > > > +		    struct irq_affinity *desc)
> > > > > > > >  {
> > > > > > > > -	return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, NULL, desc);
> > > > > > > > +	struct virtio_vq_config cfg = {};
> > > > > > > > +
> > > > > > > > +	cfg.nvqs = nvqs;
> > > > > > > > +	cfg.vqs = vqs;
> > > > > > > > +	cfg.callbacks = callbacks;
> > > > > > > > +	cfg.names = (const char **)names;
> > > > > > >
> > > > > > >
> > > > > > > Casting const away? Not safe.
> > > > > >
> > > > > >
> > > > > >
> > > > > > Because the vp_modern_create_avq() use the "const char *names[]",
> > > > > > and the virtio_uml.c changes the name in the subsequent commit, so
> > > > > > change the "names" inside the virtio_vq_config from "const char *const
> > > > > > *names" to "const char **names".
> > > > >
> > > > > I'm not sure I understand which commit you mean,
> > > > > and this kind of change needs to be documented, but it does not matter.
> > > > > Don't cast away const.
> > > >
> > > >
> > > > Do you mean change the virtio_find_vqs(), from
> > > > const char * const names[] to const char *names[].
> > > >
> > > > And update the caller?
> > > >
> > > > If we do not cast the const, we need to update all the caller to remove the
> > > > const.
> > > >
> > > > Right?
> > > >
> > > > Thanks.
> > >
> > >
> > > Just do not split the patchset at a boundary that makes you do that.
> > > If you are passing in an array from a const section then it
> > > has to be const and attempts to change it are a bad idea.
> >
> > Without this patch set:
> >
> > static struct virtqueue *vu_setup_vq(struct virtio_device *vdev,
> > 				     unsigned index, vq_callback_t *callback,
> > 				     const char *name, bool ctx)
> > {
> > 	struct virtio_uml_device *vu_dev = to_virtio_uml_device(vdev);
> > 	struct platform_device *pdev = vu_dev->pdev;
> > 	struct virtio_uml_vq_info *info;
> > 	struct virtqueue *vq;
> > 	int num = MAX_SUPPORTED_QUEUE_SIZE;
> > 	int rc;
> >
> > 	info = kzalloc(sizeof(*info), GFP_KERNEL);
> > 	if (!info) {
> > 		rc = -ENOMEM;
> > 		goto error_kzalloc;
> > 	}
> > ->	snprintf(info->name, sizeof(info->name), "%s.%d-%s", pdev->name,
> > 		 pdev->id, name);
> >
> > 	vq = vring_create_virtqueue(index, num, PAGE_SIZE, vdev, true, true,
> > 				    ctx, vu_notify, callback, info->name);
> >
> >
> > The name is changed by vu_setup_vq().
> > If we want to pass names to
> > virtio ring, the names must not be  "const char * const"
> >
> > And the admin queue of pci do the same thing.
> >
> > And I think you are right, we should not cast the const.
> > So we have to remove the "const" from the source.
> > And I checked the source code, if we remove the "const", I think
> > that makes sense.
> >
> > Thanks.
>
> /facepalm
>
> This is a different const.
>
>
> There should be no need to drop the annotation, core
> does not change these things and using const helps make
> sure that is the case.


If you do not like this, the left only way is to allocate new
memory to store the info, if the caller do not change.

In the further, maybe the caller can use the follow struct directly.

struct virtio_vq_config {
 	vq_callback_t      callback;
 	const char         *name;
 	const bool          ctx;
};

For now, we can allocate memory to change the arrays (names, callbacks..)
to the array of struct virtio_vq_config.

And the find_vqs() accepts the array of struct virtio_vq_config.

How about this?

Thanks.

>
>
>
> >
> >
> > >
> > >
> > > > >
> > > > > --
> > > > > MST
> > > > >
> > >
>