Re: [Qemu-devel] [PATCH 31/88] QMP: use g_new() family of functions

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > [PMD: more changes]
> > ---
> >  monitor.c                 | 14 +++++++-------
> >  qmp.c                     | 14 +++++++-------
> >  tests/test-qmp-commands.c | 14 +++++++-------
> >  3 files changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index fe0d1bdbb4..ea6a485f11 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -906,7 +906,7 @@ static void query_commands_cb(QmpCommand *cmd, void *opaque)
> >          return;
> >      }
> >  
> > -    info = g_malloc0(sizeof(*info));
> > +    info = g_new0(CommandInfoList, 1);
> >      info->value = g_malloc0(sizeof(*info->value));
> >      info->value->name = g_strdup(cmd->name);
> >      info->next = *list;
> 
> I'm not convinced rewriting
> 
>        LHS = g_malloc(sizeof(*LHS));
> 
> to
> 
>        LHS = g_new(T, 1);
> 
> where T is the type of LHS is worth the trouble.  The code before the
> rewrite is pretty idiomatic.  There's no possibility of integer overflow
> g_new() could avoid.  The types are obviously correct, so the additional
> type checking is quite unlikely to catch anything.  That leaves the
> consistency argument.  I'm willing to hear it, but I feel it should be
> heard in a patch series that does nothing else.

The 'obviously correct' is the dodgy part of the argument here.
How many bugs do we right that are obviously wrong?

t.c:13:20: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     struct c *pc = g_new(struct b, 1);

seems good to me.

Dave

> > @@ -1799,7 +1799,7 @@ static void hmp_wavcapture(Monitor *mon, const QDict *qdict)
> >      int nchannels = qdict_get_try_int(qdict, "nchannels", -1);
> >      CaptureState *s;
> >  
> > -    s = g_malloc0 (sizeof (*s));
> > +    s = g_new0(CaptureState, 1);
> >  
> >      freq = has_freq ? freq : 44100;
> >      bits = has_bits ? bits : 16;
> 
> This hunk is HMP, not QMP.
> 
> > @@ -1947,7 +1947,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> >          return;
> >      }
> >  
> > -    monfd = g_malloc0(sizeof(mon_fd_t));
> > +    monfd = g_new0(mon_fd_t, 1);
> >      monfd->name = g_strdup(fdname);
> >      monfd->fd = fd;
> >  
> > @@ -2110,7 +2110,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
> >          QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
> >              FdsetFdInfoList *fdsetfd_info;
> >  
> > -            fdsetfd_info = g_malloc0(sizeof(*fdsetfd_info));
> > +            fdsetfd_info = g_new0(FdsetFdInfoList, 1);
> >              fdsetfd_info->value = g_malloc0(sizeof(*fdsetfd_info->value));
> >              fdsetfd_info->value->fd = mon_fdset_fd->fd;
> >              if (mon_fdset_fd->opaque) {
> > @@ -2199,7 +2199,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> >          }
> >      }
> >  
> > -    mon_fdset_fd = g_malloc0(sizeof(*mon_fdset_fd));
> > +    mon_fdset_fd = g_new0(MonFdsetFd, 1);
> >      mon_fdset_fd->fd = fd;
> >      mon_fdset_fd->removed = false;
> >      if (has_opaque) {
> > @@ -2207,7 +2207,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> >      }
> >      QLIST_INSERT_HEAD(&mon_fdset->fds, mon_fdset_fd, next);
> >  
> > -    fdinfo = g_malloc0(sizeof(*fdinfo));
> > +    fdinfo = g_new0(AddfdInfo, 1);
> >      fdinfo->fdset_id = mon_fdset->id;
> >      fdinfo->fd = mon_fdset_fd->fd;
> >  
> > @@ -4102,7 +4102,7 @@ void monitor_init(Chardev *chr, int flags)
> >          is_first_init = 0;
> >      }
> >  
> > -    mon = g_malloc(sizeof(*mon));
> > +    mon = g_new(Monitor, 1);
> >      monitor_data_init(mon);
> >  
> >      qemu_chr_fe_init(&mon->chr, chr, &error_abort);
> 
> This is monitor core, not QMP.
> 
> > diff --git a/qmp.c b/qmp.c
> > index e8c303116a..e965020e37 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -232,7 +232,7 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
> >      while ((prop = object_property_iter_next(&iter))) {
> >          ObjectPropertyInfoList *entry = g_malloc0(sizeof(*entry));
> >  
> > -        entry->value = g_malloc0(sizeof(ObjectPropertyInfo));
> > +        entry->value = g_new0(ObjectPropertyInfo, 1);
> >          entry->next = props;
> >          props = entry;
> >  
> > @@ -432,7 +432,7 @@ static void qom_list_types_tramp(ObjectClass *klass, void *data)
> >      ObjectTypeInfo *info;
> >      ObjectClass *parent = object_class_get_parent(klass);
> >  
> > -    info = g_malloc0(sizeof(*info));
> > +    info = g_new0(ObjectTypeInfo, 1);
> >      info->name = g_strdup(object_class_get_name(klass));
> >      info->has_abstract = info->abstract = object_class_is_abstract(klass);
> >      if (parent) {
> > @@ -440,7 +440,7 @@ static void qom_list_types_tramp(ObjectClass *klass, void *data)
> >          info->parent = g_strdup(object_class_get_name(parent));
> >      }
> >  
> > -    e = g_malloc0(sizeof(*e));
> > +    e = g_new0(ObjectTypeInfoList, 1);
> >      e->value = info;
> >      e->next = *pret;
> >      *pret = e;
> > @@ -490,7 +490,7 @@ static DevicePropertyInfo *make_device_property_info(ObjectClass *klass,
> >                  return NULL;           /* no way to set it, don't show */
> >              }
> >  
> > -            info = g_malloc0(sizeof(*info));
> > +            info = g_new0(DevicePropertyInfo, 1);
> >              info->name = g_strdup(prop->name);
> >              info->type = default_type ? g_strdup(default_type)
> >                                        : g_strdup(prop->info->name);
> > @@ -502,7 +502,7 @@ static DevicePropertyInfo *make_device_property_info(ObjectClass *klass,
> >      } while (klass != object_class_by_name(TYPE_DEVICE));
> >  
> >      /* Not a qdev property, use the default type */
> > -    info = g_malloc0(sizeof(*info));
> > +    info = g_new0(DevicePropertyInfo, 1);
> >      info->name = g_strdup(name);
> >      info->type = g_strdup(default_type);
> >      info->has_description = !!description;
> > @@ -568,7 +568,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
> >              continue;
> >          }
> >  
> > -        entry = g_malloc0(sizeof(*entry));
> > +        entry = g_new0(DevicePropertyInfoList, 1);
> >          entry->value = info;
> >          entry->next = prop_list;
> >          prop_list = entry;
> > @@ -712,7 +712,7 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
> >  
> >  MemoryInfo *qmp_query_memory_size_summary(Error **errp)
> >  {
> > -    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
> > +    MemoryInfo *mem_info = g_new0(MemoryInfo, 1);
> >  
> >      mem_info->base_memory = ram_size;
> >  
> > diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> > index 904c89d4d4..e715c45a23 100644
> > --- a/tests/test-qmp-commands.c
> > +++ b/tests/test-qmp-commands.c
> > @@ -28,8 +28,8 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
> >                                Error **errp)
> >  {
> >      UserDefTwo *ret;
> > -    UserDefOne *ud1c = g_malloc0(sizeof(UserDefOne));
> > -    UserDefOne *ud1d = g_malloc0(sizeof(UserDefOne));
> > +    UserDefOne *ud1c = g_new0(UserDefOne, 1);
> > +    UserDefOne *ud1d = g_new0(UserDefOne, 1);
> >  
> >      ud1c->string = strdup(ud1a->string);
> >      ud1c->integer = ud1a->integer;
> > @@ -209,23 +209,23 @@ static void test_dealloc_types(void)
> >      UserDefOne *ud1test, *ud1a, *ud1b;
> >      UserDefOneList *ud1list;
> >  
> > -    ud1test = g_malloc0(sizeof(UserDefOne));
> > +    ud1test = g_new0(UserDefOne, 1);
> >      ud1test->integer = 42;
> >      ud1test->string = g_strdup("hi there 42");
> >  
> >      qapi_free_UserDefOne(ud1test);
> >  
> > -    ud1a = g_malloc0(sizeof(UserDefOne));
> > +    ud1a = g_new0(UserDefOne, 1);
> >      ud1a->integer = 43;
> >      ud1a->string = g_strdup("hi there 43");
> >  
> > -    ud1b = g_malloc0(sizeof(UserDefOne));
> > +    ud1b = g_new0(UserDefOne, 1);
> >      ud1b->integer = 44;
> >      ud1b->string = g_strdup("hi there 44");
> >  
> > -    ud1list = g_malloc0(sizeof(UserDefOneList));
> > +    ud1list = g_new0(UserDefOneList, 1);
> >      ud1list->value = ud1a;
> > -    ud1list->next = g_malloc0(sizeof(UserDefOneList));
> > +    ud1list->next = g_new0(UserDefOneList, 1);
> >      ud1list->next->value = ud1b;
> >  
> >      qapi_free_UserDefOneList(ud1list);
> 
> Could squash together with PATCH 79 and call the result "monitor: Use
> g_new() family of functions".
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK