virHashNew refactorings - part VI

[libvirt PATCH 00/10] virHashNew refactorings - part VI

Posted by Tim Wiederhake 2 years, 9 months ago

"virHashNew" cannot return NULL, yet we check for NULL in various places.

See https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html.

Tim Wiederhake (10):
  qemuStateInitialize: `virHashNew` cannot return NULL
  qemuMonitorGetPRManagerInfo: `virHashNew` cannot return NULL
  qemuMonitorGetPRManagerInfo: Use automatic memory management
  qemuMonitorGetPRManagerInfo: Remove superfluous `goto`s
  qemuMonitorJSONGetAllBlockJobInfo: `virHashNew` cannot return NULL
  qemuMonitorJSONGetAllBlockJobInfo: Use automatic memory management
  qemuMonitorJSONGetAllBlockJobInfo: Remove superfluous `goto`s
  testQemuGetLatestCaps: `virHashNew` cannot return NULL
  testQemuGetLatestCaps: Use automatic memory management
  testQemuGetLatestCaps: Remove superfluous `goto`s

 src/qemu/qemu_driver.c       |  3 +--
 src/qemu/qemu_monitor.c      | 13 +++----------
 src/qemu/qemu_monitor_json.c | 26 ++++++++------------------
 tests/testutilsqemu.c        | 13 +++----------
 4 files changed, 15 insertions(+), 40 deletions(-)

-- 
2.31.1

Re: [libvirt PATCH 00/10] virHashNew refactorings - part VI

Posted by Peter Krempa 2 years, 9 months ago

On Mon, Jul 19, 2021 at 11:55:44 +0200, Tim Wiederhake wrote:
> "virHashNew" cannot return NULL, yet we check for NULL in various places.
> 
> See https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html.
> 
> Tim Wiederhake (10):
>   qemuStateInitialize: `virHashNew` cannot return NULL
>   qemuMonitorGetPRManagerInfo: `virHashNew` cannot return NULL
>   qemuMonitorGetPRManagerInfo: Use automatic memory management
>   qemuMonitorGetPRManagerInfo: Remove superfluous `goto`s
>   qemuMonitorJSONGetAllBlockJobInfo: `virHashNew` cannot return NULL
>   qemuMonitorJSONGetAllBlockJobInfo: Use automatic memory management
>   qemuMonitorJSONGetAllBlockJobInfo: Remove superfluous `goto`s
>   testQemuGetLatestCaps: `virHashNew` cannot return NULL
>   testQemuGetLatestCaps: Use automatic memory management
>   testQemuGetLatestCaps: Remove superfluous `goto`s

Series:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

For any further cleanups please consider fixing similar instances (e.g.
removal of NULL check for virHashNew) on a per-file basis rather than
per function.

Same way in most cases where you switching to automatic memory freeing
on a per-function basis it's okay to include cleanup of jumps/labels. 

We mostly madated separation if you want to do it on a per-file basis.

Re: [libvirt PATCH 00/10] virHashNew refactorings - part VI

Posted by Tim Wiederhake 2 years, 9 months ago

On Mon, 2021-07-19 at 14:08 +0200, Peter Krempa wrote:
> On Mon, Jul 19, 2021 at 11:55:44 +0200, Tim Wiederhake wrote:
> > "virHashNew" cannot return NULL, yet we check for NULL in various
> > places.
> > 
> > See
> > https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html
> > .
> > 
> > Tim Wiederhake (10):
> >   qemuStateInitialize: `virHashNew` cannot return NULL
> >   qemuMonitorGetPRManagerInfo: `virHashNew` cannot return NULL
> >   qemuMonitorGetPRManagerInfo: Use automatic memory management
> >   qemuMonitorGetPRManagerInfo: Remove superfluous `goto`s
> >   qemuMonitorJSONGetAllBlockJobInfo: `virHashNew` cannot return
> > NULL
> >   qemuMonitorJSONGetAllBlockJobInfo: Use automatic memory
> > management
> >   qemuMonitorJSONGetAllBlockJobInfo: Remove superfluous `goto`s
> >   testQemuGetLatestCaps: `virHashNew` cannot return NULL
> >   testQemuGetLatestCaps: Use automatic memory management
> >   testQemuGetLatestCaps: Remove superfluous `goto`s
> 
> Series:
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> 
> For any further cleanups please consider fixing similar instances
> (e.g.
> removal of NULL check for virHashNew) on a per-file basis rather than
> per function.
> 
> Same way in most cases where you switching to automatic memory
> freeing
> on a per-function basis it's okay to include cleanup of jumps/labels.
> 
> We mostly madated separation if you want to do it on a per-file
> basis.
> 

I have 38 patches still in my queue.

6 of those are two "cannot return null; use g_auto*; remove goto"
sequences that I would like to keep separate. The remaining patches are
all of the "cannot return null" kind and spread over the entire code
base. Applying them on a per-file basis would reduce the number of
patches by only four, from 32 to 28.

I am aware that this refactoring is quite noisy, but I do not think
that grouping by file makes it significantly better. What I can offer
is grouping by directory / subsystem (e.g. conf, nwfilter, qemu, util,
etc.), or sending out all remaining patches in one go, so that the
amount of mails is the same, but the process is hopefully faster.

Your thoughts?

Tim

Re: [libvirt PATCH 00/10] virHashNew refactorings - part VI

Posted by Peter Krempa 2 years, 9 months ago

On Mon, Jul 19, 2021 at 14:36:59 +0200, Tim Wiederhake wrote:
> On Mon, 2021-07-19 at 14:08 +0200, Peter Krempa wrote:
> > On Mon, Jul 19, 2021 at 11:55:44 +0200, Tim Wiederhake wrote:
> > > "virHashNew" cannot return NULL, yet we check for NULL in various
> > > places.
> > > 
> > > See
> > > https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html
> > > .
> > > 
> > > Tim Wiederhake (10):
> > >   qemuStateInitialize: `virHashNew` cannot return NULL
> > >   qemuMonitorGetPRManagerInfo: `virHashNew` cannot return NULL
> > >   qemuMonitorGetPRManagerInfo: Use automatic memory management
> > >   qemuMonitorGetPRManagerInfo: Remove superfluous `goto`s
> > >   qemuMonitorJSONGetAllBlockJobInfo: `virHashNew` cannot return
> > > NULL
> > >   qemuMonitorJSONGetAllBlockJobInfo: Use automatic memory
> > > management
> > >   qemuMonitorJSONGetAllBlockJobInfo: Remove superfluous `goto`s
> > >   testQemuGetLatestCaps: `virHashNew` cannot return NULL
> > >   testQemuGetLatestCaps: Use automatic memory management
> > >   testQemuGetLatestCaps: Remove superfluous `goto`s
> > 
> > Series:
> > 
> > Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> > 
> > For any further cleanups please consider fixing similar instances
> > (e.g.
> > removal of NULL check for virHashNew) on a per-file basis rather than
> > per function.
> > 
> > Same way in most cases where you switching to automatic memory
> > freeing
> > on a per-function basis it's okay to include cleanup of jumps/labels.
> > 
> > We mostly madated separation if you want to do it on a per-file
> > basis.
> > 
> 
> I have 38 patches still in my queue.
> 
> 6 of those are two "cannot return null; use g_auto*; remove goto"
> sequences that I would like to keep separate. The remaining patches are
> all of the "cannot return null" kind and spread over the entire code
> base. Applying them on a per-file basis would reduce the number of
> patches by only four, from 32 to 28.

In case you have the patches ready it's better to send them at once. The
risk of having to change a lot of stuff is at worst the same.