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

[Parav Pandit <parav@nvidia.com>]

> From: Max Gurtovoy <mgurtovoy@nvidia.com>
> Sent: Monday, March 6, 2023 6:05 AM
> 
> 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.
> 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).

This is also a good proposal that 3 positives.

1. Tells exactly what was wrong with command or with device
2. generic enough to convert into abstract OS-specific error codes with negligible cost (switch-case)
3. can define command-specific error details