Re: [PATCH v2 05/18] monitor: Introduce monitor_fdset_*free

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 23, 2024 at 04:05:35PM -0300, Fabiano Rosas wrote:
>> Introduce two new functions to remove and free no longer used fds and
>> fdsets.
>> 
>> We need those to decouple the remove/free routines from
>> monitor_fdset_cleanup() which will go away in the next patches.
>> 
>> The new functions:
>> 
>> - monitor_fdset_free() will be used when a monitor connection closes
>>   and when an fd is removed to cleanup any fdset that is now empty.
>> 
>> - monitor_fdset_fd_free() will be used to remove one or more fds that
>>   have been explicitly targeted by qmp_remove_fd().
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> One nitpick below.
>
>> ---
>>  monitor/fds.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>> 
>> diff --git a/monitor/fds.c b/monitor/fds.c
>> index fb9f58c056..101e21720d 100644
>> --- a/monitor/fds.c
>> +++ b/monitor/fds.c
>> @@ -167,6 +167,22 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>>      return -1;
>>  }
>>  
>> +static void monitor_fdset_free(MonFdset *mon_fdset)
>> +{
>> +    if (QLIST_EMPTY(&mon_fdset->fds) && QLIST_EMPTY(&mon_fdset->dup_fds)) {
>> +        QLIST_REMOVE(mon_fdset, next);
>> +        g_free(mon_fdset);
>> +    }
>> +}
>
> Would monitor_fdset_free_if_empty() (or similar) slightly better?

Yes, I'll use that.

>
> static void monitor_fdset_free(MonFdset *mon_fdset)
> {
>     QLIST_REMOVE(mon_fdset, next);
>     g_free(mon_fdset);
> }
>
> static void monitor_fdset_free_if_empty(MonFdset *mon_fdset)
> {
>     if (QLIST_EMPTY(&mon_fdset->fds) && QLIST_EMPTY(&mon_fdset->dup_fds)) {
>         monitor_fdset_free(mon_fdset);
>     }
> }
>
>> +
>> +static void monitor_fdset_fd_free(MonFdsetFd *mon_fdset_fd)
>> +{
>> +    close(mon_fdset_fd->fd);
>> +    g_free(mon_fdset_fd->opaque);
>> +    QLIST_REMOVE(mon_fdset_fd, next);
>> +    g_free(mon_fdset_fd);
>> +}
>> +
>>  static void monitor_fdset_cleanup(MonFdset *mon_fdset)
>>  {
>>      MonFdsetFd *mon_fdset_fd;
>> @@ -176,17 +192,11 @@ static void monitor_fdset_cleanup(MonFdset *mon_fdset)
>>          if ((mon_fdset_fd->removed ||
>>                  (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) &&
>>                  runstate_is_running()) {
>> -            close(mon_fdset_fd->fd);
>> -            g_free(mon_fdset_fd->opaque);
>> -            QLIST_REMOVE(mon_fdset_fd, next);
>> -            g_free(mon_fdset_fd);
>> +            monitor_fdset_fd_free(mon_fdset_fd);
>>          }
>>      }
>>  
>> -    if (QLIST_EMPTY(&mon_fdset->fds) && QLIST_EMPTY(&mon_fdset->dup_fds)) {
>> -        QLIST_REMOVE(mon_fdset, next);
>> -        g_free(mon_fdset);
>> -    }
>> +    monitor_fdset_free(mon_fdset);
>>  }
>>  
>>  void monitor_fdsets_cleanup(void)
>> -- 
>> 2.35.3
>>