[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v10 03/10] admin: introduce group administration commands
On 03/03/2023 15:19, Michael S. Tsirkin wrote:
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.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. StefanReally 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. StefanIt's 2:2 for now, you are with Parav, me and Cornelia like it :) Or I will try to document it better.
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_MEMBER5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the command_specific_error field).
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]