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

[Cornelia Huck <cohuck@redhat.com>]

On Tue, Apr 18 2023, Halil Pasic <pasic@linux.ibm.com> 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.

Using "notif" throughout would probably be most consistent.