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

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

On Mon, Mar 06, 2023 at 01:04:42PM +0200, Max Gurtovoy wrote:
> 
> 
> On 03/03/2023 15:19, Michael S. Tsirkin wrote:
> > On Fri, Mar 03, 2023 at 08:17:03AM -0500, Stefan Hajnoczi wrote:
> > > On Thu, Mar 02, 2023 at 07:01:56PM -0500, Michael S. Tsirkin wrote:
> > > > On Thu, Mar 02, 2023 at 03:19:12PM -0500, Stefan Hajnoczi wrote:
> > > > > On Thu, Mar 02, 2023 at 06:40:29PM +0000, Parav Pandit wrote:
> > > > > > 
> > > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > Sent: Thursday, March 2, 2023 8:05 AM
> > > > > > 
> > > > > > > +When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
> > > > > > > +is reserved and set to zero by the device.
> > > > > > > +
> > > > > > s/status_qialifier/status_qualifier
> > > > > > Missed from v10 of Feb.
> > > > > > 
> > > > > > > +When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
> > > > > > > +describes possible \field{status_qialifier} values:
> > > > > > s/status_qialifier/status_qualifier
> > > > > > 
> > > > > > Can you please add other useful error codes in addition to the EINVAL?
> > > > > > Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.
> > > > > 
> > > > > Please define a unique constant for each error condition that can occur
> > > > > instead of sharing catch-all errno constants between multiple error
> > > > > conditions. If a driver wants to squash them together into an errno,
> > > > > that's fine, but I think doing this at the hardware interface level is
> > > > > just propagating the mistakes of errnos.
> > > > > 
> > > > > Only status_qualifier is needed and the vague status field can be
> > > > > dropped. It's not clear to me why adding EAGAIN, ENOMEM, EBUSY, and
> > > > > ENODEV is useful. They have no meaning to the driver, only the
> > > > > status_qualifier really indicates what is going on.
> > > > 
> > > > At a high level at the moment we have only two cases:
> > > > - ok
> > > > - invalid input supplied by driver
> > > > 
> > > > maybe we will have more reasons for a failure - remains to
> > > > be seen.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > I'm sure you guys have discussed this previously, but please provide
> > > > > rationale in the spec because it looks weird to someone with fresh eyes.
> > > > > 
> > > > > Stefan
> > > > 
> > > > Really most drivers just want to propagate errno to userspace.
> > > > All the detailed reporting is for sure well intentional but
> > > > in the end it is at best printed into log - end to end
> > > > people just end up with a switch statement
> > > > converting these to errno codes.
> > > > So we are passing them from device and this way there will be
> > > > some uniformity.
> > > 
> > > Please clarify the rationale in the spec. I don't agree with it, as
> > > explained in my earlier email, but as long as its documented, people can
> > > try to follow it.
> > > 
> > > Stefan
> > 
> > It's 2:2 for now, you are with Parav, me and Cornelia like it :)
> > Or I will try to document it better.
> I don't understand this status_qualifier as well and it wasn't included in
> my original patch set.

Sounds like you feel I should drop your S.O.B - is this the complaint?
I wanted to give attribution since I started with that but sure, no
problem.

> I vote for "status" that describe generic status codes and
> "command_specific_error" that should be inspected by the driver only if
> "status" is set to "VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR".
> We discussed this so many times before (and already agreed IIRC) and adding
> this new qualifiers mechanism sounds not right to me and not intuitive for
> device and driver developers.
> 
> I suggest:
> 
> 1. VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE
> 2. VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD
> 3. VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP
> 4. VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER
> 5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the
> command_specific_error field).

I don't think it's a good idea, we'll have to agree to disagree.


The point is simple. We can have a detailed virtio specific error.
Nice for debugging most drivers won't know what to do with it.
This is the status_qualifier.
Very detailed but
generally drivers will just have a giant switch statement translating
it to a simple error code for userspace.
So to save everyone work, we also added "status"
a generic kind of error class that is easy to pass to userspace
with a small switch statement.

COMMAND_SPECIFIC_ERR is just way too much detail - commands generally
just should not fail it's a quality of implementation issue.



> 
> 
> >