[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 06/10] monitor: release the lock before calling close()
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 06/10] monitor: release the lock before calling close() |
Date: |
Mon, 06 Mar 2023 16:29:26 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
Alex Bennée <alex.bennee@linaro.org> writes:
> marcandre.lureau@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> As per comment, presumably to avoid syscall in critical section.
>>
>> Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros")
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> I know this is already merged but as an academic exercise we could have
> kept the lock guard with a little restructuring like this:
>
> void qmp_closefd(const char *fdname, Error **errp)
> {
> Monitor *cur_mon = monitor_cur();
> mon_fd_t *monfd;
> int tmp_fd = -1;
>
> WITH_QEMU_LOCK_GUARD(&cur_mon->mon_lock) {
> QLIST_FOREACH(monfd, &cur_mon->fds, next) {
No need for QLIST_FOREACH_SAFE(), because ....
> if (strcmp(monfd->name, fdname) != 0) {
> continue;
> }
>
> QLIST_REMOVE(monfd, next);
> tmp_fd = monfd->fd;
> g_free(monfd->name);
> g_free(monfd);
> break;
... we break the loop right after modifying the list. Correct?
> }
> }
>
> if (tmp_fd > 0) {
You mean >=
> /* close() must be outside critical section */
> close(tmp_fd);
> } else {
> error_setg(errp, "File descriptor named '%s' not found", fdname);
This error is new. See also my reply to v4 of this patch.
> }
If we don't need the new error, we could simply
close(tmp_fd);
unconditionally.
> }
>
> To my mind it makes it easier to reason about locking but I probably
> have an irrational aversion to multiple exit paths for locks.
My (possibly irrational) aversion is to
if (good) {
do some more
} else {
handle error
}
I prefer
if (!good) {
handle error
bail out
}
do some more