Re: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names

[Halil Pasic <pasic@linux.ibm.com>]

On Mon, 17 Apr 2023 20:53:32 +0000
Parav Pandit <parav@nvidia.com> wrote:

> > From: Halil Pasic <pasic@linux.ibm.com>
> > Sent: Sunday, April 16, 2023 11:42 PM  
[..]
> >   
> > > @@ -1053,9 +1059,9 @@ \subsubsection{Available Buffer
> > > Notifications}\label{sec:Virtio Transport Option  If  
> > VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:  
> > >  \begin{itemize}
> > >  \item If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the
> > > driver MUST use the -\field{queue_notify_data} value instead of the virtqueue  
> > index.  
> > > +\field{queue_notify_id} value instead of the virtqueue index.
> > >  \item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver
> > > MUST set the -\field{vqn} field to the \field{queue_notify_data} value.
> > > +\field{vq_notify_id} field to the \field{queue_notify_id} value.  
> > 
> > Seems you did not catch these with the s/_id/_config_data/. By doing the
> > replace one would get queue_notify_config_data, vq_notify_config_data and
> > queue_notify_config_data respectively.
> > 
> > But that still does not seem right, and the right answer depends on to what do
> > we rename \field{vqn}.
> >   
> vqn -> vq_notif_config_data in the notification structure.

I think I got what you mean. I intend to double check it
in v14.

> 
> virtio_pci_common_cfg.queue_notify_data -> queue_notify_config_data.
> 

I have no strong opinion about this one.

> Better to rename
> 
> VIRTIO_F_NOTIF_CONFIG_DATA to VIRTIO_F_NOTIFY_CONFIG_DATA.
> Adding "Y" is not going to make it that long. Field names will have some consistency.
> WDYT?

No strong opinion on this either. I agree with Michael that "NOTIF"
likely stands for "notification", but I also see your point: having
"notify" in the field names and "NOTIF" in the feature name isn't great
either.

IMHO VIRTIO_F_NOTIFY_CONFIG_DATA should remain aligned with
VIRTIO_F_NOTIFICATION_DATA in one way or another. Thus way out would be
to do something like:
virtio_pci_common_cfg.queue_notify_data -> queue_notif_config_data

I think as editors Connie and Michael should have the most say in this
though.

Regards,
Halil


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org