On Wednesday, 2021-01-13 at 13:26:48 +03, Vladimir Sementsov-Ogievskiy wrote:
> 12.01.2021 18:27, David Edmondson wrote:
>> When a call to fcntl(2) for the purpose of manipulating file locks
>> fails with an error other than EAGAIN or EACCES, report the error
>> returned by fcntl.
>>
>> EAGAIN or EACCES are elided as they are considered to be common
>> failures, indicating that a conflicting lock is held by another
>> process.
>>
>> Signed-off-by: David Edmondson <david.edmond...@oracle.com>
>> ---
>> v3:
>> - Remove the now unnecessary updates to the test framework (Max).
>> - Elide the error detail for EAGAIN or EACCES when locking (Kevin,
>> sort-of Max).
>> - Philippe and Vladimir sent Reviewed-by, but things have changed
>> noticeably, so I didn't add them (dme).
>>
>> v4:
>> - Really, really remove the unnecessary updates to the test framework.
>>
>> block/file-posix.c | 52 +++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 42 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 00cdaaa2d4..c5142f7ffa 100644
>
> Hmm, not it looks like too much code duplication. Maybe, we can add a helper
> macro, like
>
> #define raw_lock_error_setg_errno(errp, os_error, fmt, ...) \
> do {
> if (err == EAGAIN || err == EACCES) {
> error_setg((errp), (fmt), ## __VA_ARGS__);
> } else {
> error_setg_errno((errp), (os_error), (fmt), ## __VA_ARGS__);
> }
> } while (0)
>
> We can't make a helper function instead, as error_setg_errno is already a
> macro and it wants to use __LINE__..
>
> But I think that macro is better than duplication anyway.
Okay.
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -836,7 +836,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
>> if ((perm_lock_bits & bit) && !(locked_perm & bit)) {
>> ret = qemu_lock_fd(fd, off, 1, false);
>> if (ret) {
>> - error_setg(errp, "Failed to lock byte %d", off);
>> + int err = -ret;
>> +
>> + if (err == EAGAIN || err == EACCES) {
>> + error_setg(errp, "Failed to lock byte %d", off);
>> + } else {
>> + error_setg_errno(errp, err, "Failed to lock byte %d",
>> off);
>> + }
>> return ret;
>> } else if (s) {
>> s->locked_perm |= bit;
>> @@ -844,7 +850,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
>> } else if (unlock && (locked_perm & bit) && !(perm_lock_bits &
>> bit)) {
>> ret = qemu_unlock_fd(fd, off, 1);
>> if (ret) {
>> - error_setg(errp, "Failed to unlock byte %d", off);
>> + int err = -ret;
>> +
>> + if (err == EAGAIN || err == EACCES) {
>> + error_setg(errp, "Failed to lock byte %d", off);
>
> s/lock/unlock
>
>> + } else {
>> + error_setg_errno(errp, err, "Failed to lock byte %d",
>> off);
>
> and here.
>
> Which proves, that code duplication is a bad idea in general :)
Ouch. Will fix.
>
>> + }
>> return ret;
>> } else if (s) {
>> s->locked_perm &= ~bit;
>> @@ -857,7 +869,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
>> if ((shared_perm_lock_bits & bit) && !(locked_shared_perm & bit)) {
>> ret = qemu_lock_fd(fd, off, 1, false);
>> if (ret) {
>> - error_setg(errp, "Failed to lock byte %d", off);
>> + int err = -ret;
>> +
>> + if (err == EAGAIN || err == EACCES) {
>> + error_setg(errp, "Failed to lock byte %d", off);
>> + } else {
>> + error_setg_errno(errp, err, "Failed to lock byte %d",
>> off);
>> + }
>> return ret;
>> } else if (s) {
>> s->locked_shared_perm |= bit;
>> @@ -866,7 +884,7 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
>> !(shared_perm_lock_bits & bit)) {
>> ret = qemu_unlock_fd(fd, off, 1);
>> if (ret) {
>> - error_setg(errp, "Failed to unlock byte %d", off);
>> + error_setg_errno(errp, -ret, "Failed to unlock byte %d",
>> off);
>
> Don't know, why same logic is not applied here.. Probably I've missed from
> the previous discussion. Anyway, if it is intentional exclusion, not bad to
> mention it in commit message.
>From the documentation, it didn't seem to make sense that the unlock
case would generate EAGAIN or EACCES for "someone is already holding the
lock", so neither should be elided.
I can add a note in the commit message.
>> return ret;
>> } else if (s) {
>> s->locked_shared_perm &= ~bit;
>> @@ -890,9 +908,16 @@ static int raw_check_lock_bytes(int fd, uint64_t perm,
>> uint64_t shared_perm,
>> ret = qemu_lock_fd_test(fd, off, 1, true);
>> if (ret) {
>> char *perm_name = bdrv_perm_names(p);
>> - error_setg(errp,
>> - "Failed to get \"%s\" lock",
>> - perm_name);
>> + int err = -ret;
>> +
>> + if (err == EAGAIN || err == EACCES) {
>> + error_setg(errp, "Failed to get \"%s\" lock",
>> + perm_name);
>
> fit in one line
>
>> + } else {
>> + error_setg_errno(errp, err,
>> + "Failed to get \"%s\" lock",
>> + perm_name);
>
> fit in two lines..
>
>> + }
>> g_free(perm_name);
>> return ret;
>> }
>> @@ -905,9 +930,16 @@ static int raw_check_lock_bytes(int fd, uint64_t perm,
>> uint64_t shared_perm,
>> ret = qemu_lock_fd_test(fd, off, 1, true);
>> if (ret) {
>> char *perm_name = bdrv_perm_names(p);
>> - error_setg(errp,
>> - "Failed to get shared \"%s\" lock",
>> - perm_name);
>> + int err = -ret;
>> +
>> + if (err == EAGAIN || err == EACCES) {
>> + error_setg(errp, "Failed to get shared \"%s\" lock",
>> + perm_name);
>> + } else {
>> + error_setg_errno(errp, err,
>> + "Failed to get shared \"%s\" lock",
>> + perm_name);
>> + }
>> g_free(perm_name);
>> return ret;
>> }
>>
>
> also, I don't see much benefit in creating additional "err" variable instead
> of just use ret, but it's a kind of taste..
Okay.
dme.
--
Tonight I'm gonna bury that horse in the ground.
