On 8/29/19 12:02 PM, Daniel P. Berrangé wrote: > See this previous thread: > > https://www.redhat.com/archives/libvir-list/2019-May/msg00273.html > > The executive summary is that catching & reporting ENOMEM is not worth > the maint cost because: > > - this code will almost never run on Linux hosts > > - if it does run it will likely have bad behaviour silently dropping > data or crashing the process > > - apps using libvirt often do so via a non-C language that aborts/exits > the app on OOM regardless, or use other C libraries that abort > > - we can build a system that is more reliable when OOM happens by > not catching OOM, instead simply letting apps exit, restart and > carry on where they left off > > The first part of the series does the bare minimum to cull OOM handling. > > With this done, the main reason why we never adopted glib is now > removed. Thus the second part of this series introduces use of glib into > libvirt and converts our our allocation APIs to use the glib allocation > APIs internally. > > In future I'd especially like to use glib to replace virObject code, > which I knowingly wrote as a somewhat pathetic clone of GObject. > Eliminating all our DBus code is also another thing I'd like, since > Glib's DBus client code is better IMHO. > > Note that none of the callers are updated at this point, so they are all > still attempting to handle errors from VIR_ALLOC, VIR_STRDUP, etc. If > we convert VIR_ALLOC calls to remove return value checks, and then later > convert to glib's g_new0 API, we go through two lots of churn which I > think is undesirable. > > Thus I think it is highly desirable to introduce glib straight away and > do a single conversion step. It also means we can introduce other data > structures from glib to replace ours and avoid wasting time converting > those too. > > Overall the code in this series is all quite simple and a nice cleanup. > 50% of the lines culled come from the first patch removing OOM testing, > the other 50% come from viralloc.c impl simplification > > The interesting question is the impact is has on downstreams who have > to backport patches to older versions. > > If we start converting callers to g_new0, etc, then downstream have > to either > > * Change g_new0 back into VIR_ALLOC and likely re-add many goto > calls we purged > > Or > > * Backport all the patches in this series that drop OOM handling > and introduce glib usage > > If we start adopting other glib features beyond g_new0, then downstreams > are pretty much forced into option 2. > > Given the benefit I think we'll see from this series in our codebase, > I'm inclined to say we should prioritize the future, over prioritizing > the past (backports), and thus freely adopt use of glib APIs rightaway. > > IOW, I think we should expect vendors to backport this series as a > starting point, before any other patches. I've not tested but I'm > hopeful that such a backport of this series is fairly easy. The > viralloc.{c,h} file hasn't seen much change in recent times so > ought to be a clean cherry-pick. The glib additions might hit > some conflicts in makefiles, but not too terrible I hope. Probably > worth doing a test to see just how easy backports are over the past > 1 year of releases.
Given the primary maintenance burden I'll be seeing over the next years is on versions 5.1.0 and 4.0.0, I took at stab at backporting this series to those releases. Here are some notes I scribbled while backporting to 5.1.0: Patch1 has conflicts due to moving of virtauto out of viralloc.h by commit a4bfc252. Patch2 has conflicts, mostly due to whitespace changes from switching to '#pragma once'. E.g. commit a6d438a9 in the case of viralloc.h Patch3 has similar conflicts to 2. Too bad you have to patch functions (that have conflicts) in patch 2 that are removed in this patch. Patch4 has conflicts due to '#pragma once' Patch5 applies cleanly! Patch6 has conflicts due to '#pragma once' Patch7 applies cleanly! I then took the refreshed patches and tried applying to 4.0.0: Patch1 has conflicts due to no autoptr stuff in 4.0.0 and due to some changes in testutils.c (hunk 5). Patch2 and patch3 apply cleanly! Patch4 has conflicts in hunk 5 of virstring.h, but I couldn't spot cause of conflict. Patch5 has some fuzz due to introduction of *_FIREWALLD_ZONE Patch6 has conflicts since there are no */Makefile.inc.am in 4.0.0 Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
