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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Apr 18, 2023 at 01:39:58PM +0200, Halil Pasic wrote:
> 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

Fine by me.

-- 
MST