Re: [PATCH v3 06/10] monitor: release the lock before calling close()

[Markus Armbruster]

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)

Marc-André is patching qmp_getfd(), not qmp_closefd().

>   {
>       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) {
>               if (strcmp(monfd->name, fdname) != 0) {
>                   continue;
>               }
>
>               QLIST_REMOVE(monfd, next);
>               tmp_fd = monfd->fd;
>               g_free(monfd->name);
>               g_free(monfd);
>               break;
>           }
>       }
>
>       if (tmp_fd > 0) {
>           /* close() must be outside critical section */
>           close(tmp_fd);
>       } else {
>           error_setg(errp, "File descriptor named '%s' not found", fdname);
>       }
>   }
>
> To my mind it makes it easier to reason about locking but I probably
> have an irrational aversion to multiple exit paths for locks.