Re: [PATCH v10 03/10] admin: introduce group administration commands

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

On Tue, Feb 14, 2023 at 01:18:42AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Sunday, February 12, 2023 4:50 AM
> > 
> > On Sat, Feb 11, 2023 at 06:50:37PM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Thursday, February 9, 2023 7:14 AM
> > >
> > > [..]
> > >
> > > > +Group administration commands can be issued through an owner device
> > > > +to control member devices of a group.
> > > can be issued by the owner device to control ...
> > 
> > Well it's clearly the driver that issues the commands. Don't see how
> > can a device issue them. If instead of "through an owner device"
> > you prefer a more verbose "the driver of an owner device" then
> > I can live with that. Let me know.
> 
> "through" wording is convoluted and it hints that someone else is preparing the command and its tunneled through the admin vq.
> Which is clearly not the scope here.

It is exactly someone else - the driver not the owner itself.

> 
> How about below simple statements like below?
> 
> Group administrative commands control the member devices.

> The driver sends group administrative commands to the owner device.

The point is that it's the same group though which this misses.
I think something like:

	The driver sends group administration commands to the owner device of
	the group to control member devices of the group.

will do the trick.

> 
> > > General spec structure is describing the "rules" as device and driver side
> > requirements.
> > > Can you please move above rule paragraph belongs to device and driver
> > requirements section?
> > 
> > Generally we have a bit of duplication. A high level description
> > showing how things work together. Then per-device/driver conforumance
> > statements. So I think this should stay but I will look at adding
> > conformance clauses too.
> Ok. that will be good.
> Yes. General description is good without the "rule" wording so that we have clear separation of conformance and description.

OK I will consider replacing "General rule" with "Generally". Not sure why it is better:
conformance uses rfc2119 which does not include any wording about rule
but oh well.


> > > I am not sure it should be intentional and mention of Linux.
> > > Any OS would need multiple error values to know the error cause.
> > > Linux doesn't have name "OK" either for well-known value of 0.
> > >
> > > For example
> > > $ errno 22
> > > EINVAL 22 Invalid argument
> > >
> > > $ errno 0
> > > This has not output for it and doesn't exist in errno.h.
> > 
> > No name OK true, but it does use 0 to signal success sometimes:
> Yes, but the wording is "success" and not "ok". :)

Hmm. A but wordy and we have _OK elsewhere in the spec, while Linux
does not have either OK ot SUCCESS. Just informal text.
Will consider how to make it clearer.

> > 
> > https://man7.org/linux/man-pages/man3/errno.3.html
> > 
> > For some system calls and library functions (e.g.,
> >        getpriority(2)), -1 is a valid return on success.  In such cases,
> >        a successful return can be distinguished from an error return by
> >        setting errno to zero before the call, and then, if the call
> >        returns a status that indicates that an error may have occurred,
> >        checking to see if errno has a nonzero value.
> > 
> > 
> > 
> > > Description is already good enough to describe what they are.
> > > Can we please drop Linux wording?
> > 
> > But why should we? 
> Because of following reasons.
> 
> 1. If we start putting 22 with Linux annotations, in few weeks virtio Net device section will be full of annotation of ethtool, tc, ip config and more for Linux developers.
> For example, the current two interrupt moderation patches need to write ethtool options details to match to following this Linux example here.
> It is better to avoid such things and keep spec OS agnostics, even though it is addressing and fitting into the Linux use cases.
> 
> 2. Virtio error code to Linux error code switch-case is simple routine to have.
> 
> 3. Every time virtio spec to refer to errno.h for finding right value is opposite of what spec may want to achieve.
> If an error code doesn't match to errno.h, now spec developers and reviewers to review what is not defined by errno.h to find as unique value.
> 
> 3.1 And if that is taken in future by errno.h for something else?
> 
> All of this is not worth a simple switch case statement to deal with.

I think we'll have to agree to disagree here. Years working on virt
taught me that matching some existing interface is usually better than
coming up with our own. Even if it is a bit more work upfront it pays
dividends later. Forcing some discipline in not coming up with
outlandish "EFOOBAR" codes no one driver will end up using is a good idea too.
And yes we can come up with crazy conventions like 0xdead for success.
Does not mean we should.

-- 
MST