On Sun, Oct 30, 2022 at 06:05:35PM -0500, Mike Christie wrote:
> The problem I hit is that in the ioctl code I then have to do:
>
> @@ -269,7 +270,14 @@ static int blkdev_pr_register(struct block_device *bdev,
>
> if (reg.flags & ~PR_FL_IGNORE_KEY)
> return -EOPNOTSUPP;
> - return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
> + ret = ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
> + if (ret == -EBADE) {
> + if (bdev_is_nvme(bdev))
> + ret = NVME_SC_RESERVATION_CONFLICT;
> + else if (bdev_is_scsi(bdev)
> + ret = SAM_STAT_RESERVATION_CONFLICT;
> + }
> + return ret;
Eww. We should have never leaked protocol specific values out to
userspace. This is an original bug I introduceѕ, so all blame is on me.
I suspect the right way to fix is is to keep the numeric value of
SAM_STAT_RESERVATION_CONFLICT and give it a new constant exposed in
the uapi header, assuming that SCSI is the thing people actually
used the PR API for, and nvme was just an nice little add-on.
Now if an errno value or blk_status_t is returned from the method
should not matter for this fix, but in the long run I think the
blk_status_t would be cleaner than the int used for errno, and
that will also prevent us from returning accidental non-intended
values.
Btw, I also thing we should rename BLK_STS_NEXUS to
BLK_STS_RESERVATION_CONFLICT (assuming s390 is ok with that), as that
has much better documentary value.
> + case BLK_STS_TRANSPORT:
> + if (bdev_is_nvme(bdev))
> + ret = NVME_SC_HOST_PATH_ERROR;
> + else if (bdev_is_scsi(bdev)
> + ret = DID_TRANSPORT_FAILFAST or DID_TRANSPORT_MARGINAL;
> + break;
And we'll need an uapi value for this as well.
> + case BLK_STS_NOTSUPP:
and maybe this unless we can just get away with the negative errno
value.
