As pointed out by Ján Tomko, "no_memory seems suspicious in the times of
abort()".

As libvirt decided to take the path to not report OOM and simply abort
when it happens, let's get rid of the no_memory labels and simplify the
code around them.

The two exceptions are:
- phyp code, as libvirt may end up dropping this code entirely;
- virfirewall.c code, as it seems we heavily really on firewall->err
  being set to ENOMEM;

If one thinks that virfirewall.c should also be converted, please, shout
out and I'll work on that.

Fabiano Fidêncio (5):
  conf: Get rid of "no_memory" labels
  openvz: Get rid of "no_memory" labels
  rpc: Get rid of "no_memory" labels
  util: Get rid of "no_memory" labels
  vbox: Get rid of "no_memory" labels

 src/conf/capabilities.c  | 16 ++--------
 src/conf/domain_audit.c  | 40 +++++++------------------
 src/openvz/openvz_conf.c | 18 +++++------
 src/rpc/virnetclient.c   | 42 ++++++++------------------
 src/util/virsysinfo.c    | 64 +++++++++++++++-------------------------
 src/util/virsysinfo.h    |  2 ++
 src/util/viruri.c        | 28 +++++++-----------
 src/vbox/vbox_common.c   | 20 +++++--------
 8 files changed, 75 insertions(+), 155 deletions(-)

-- 
2.24.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 12/20/19 7:43 AM, Fabiano Fidêncio wrote:
> As pointed out by Ján Tomko, "no_memory seems suspicious in the times of
> abort()".
> 
> As libvirt decided to take the path to not report OOM and simply abort
> when it happens, let's get rid of the no_memory labels and simplify the
> code around them.
> 

Series:
Reviewed-by: Cole Robinson <crobinso@redhat.com>

> The two exceptions are:
> - phyp code, as libvirt may end up dropping this code entirely;
> - virfirewall.c code, as it seems we heavily really on firewall->err
>   being set to ENOMEM;
> 

I looked at it a bit. It can probably all be ripped out but it's a
little convoluted. virCommand seems to have some similar ENOMEM handling
as well.

I think a nice prep step that will simplify this style of cleanups, is
to drop the return value from the VIR_*LLOC* macros. After the glib
conversion, they always return 0, or abort. But everywhere in the code
is still checking for 'if (VIR_ALLOC(foo) < 0)' and similar.

Long term we should replace that with g_new0 but it's not a drop in
replacement. An easy intermediate step we can do is entirely drop the '<
0' checking. This will removal a lot of 'if' conditionals that would
need to be tweaked if we work on dropping cleanup: labels now. It could
be mass done per directory and outside of a few cases I think they would
all be trivial.

I think after a step like that, there would be many util/ functions that
never return error, which is another thing to unwind up the chain.

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Dec 20, 2019 at 03:29:42PM -0500, Cole Robinson wrote:
> On 12/20/19 7:43 AM, Fabiano Fidêncio wrote:
> > As pointed out by Ján Tomko, "no_memory seems suspicious in the times of
> > abort()".
> > 
> > As libvirt decided to take the path to not report OOM and simply abort
> > when it happens, let's get rid of the no_memory labels and simplify the
> > code around them.
> > 
> 
> Series:
> Reviewed-by: Cole Robinson <crobinso@redhat.com>
> 
> > The two exceptions are:
> > - phyp code, as libvirt may end up dropping this code entirely;
> > - virfirewall.c code, as it seems we heavily really on firewall->err
> >   being set to ENOMEM;
> > 
> 
> I looked at it a bit. It can probably all be ripped out but it's a
> little convoluted. virCommand seems to have some similar ENOMEM handling
> as well.
> 
> I think a nice prep step that will simplify this style of cleanups, is
> to drop the return value from the VIR_*LLOC* macros. After the glib
> conversion, they always return 0, or abort. But everywhere in the code
> is still checking for 'if (VIR_ALLOC(foo) < 0)' and similar.
> 
> Long term we should replace that with g_new0 but it's not a drop in
> replacement. An easy intermediate step we can do is entirely drop the '<
> 0' checking. This will removal a lot of 'if' conditionals that would
> need to be tweaked if we work on dropping cleanup: labels now. It could
> be mass done per directory and outside of a few cases I think they would
> all be trivial.

When we first introduced the use of glib we declared that we should *not*
simply remove the '< 0' from VIR_ALLOC statements, because this will double
the churn in the code base by making it a 2 step conversion. We want to go
straight to g_new0, which is why we kept the "ATTRIBUTE_RETURN_CHECK"
annotation on VIR_ALLOC & friends.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list