Re: [PATCH v3 20/27] conf: add a virFirewall object to virNetworkObj

On Thu, Apr 25, 2024 at 01:38:26AM -0400, Laine Stump wrote: > This virFirewall object will store the list of actions required to > remove the firewall that was added for the currently active instance > of the network, so it has been named "fwRemoval" (and when parsed into > XML, the <firewall> element will have the name "fwRemoval"). > > There are no uses of the fwRemoval object in the virNetworkObj yet, > but everything is in place to add it to the XML when formatted, parse > it from the XML when reading network status, and free the virFirewall > object when the virNetworkObj is freed. > > Signed-off-by: Laine Stump <laine(a)redhat.com&gt; > Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > Change from V2: > * add name='fwRemoval' to the network status <firewall> so it > can be positively identified in the XML > > src/conf/virnetworkobj.c | 46 ++++++++++++++++++++++++++++++++++++++++ > src/conf/virnetworkobj.h | 11 ++++++++++ > src/libvirt_private.syms | 3 +++ > 3 files changed, 60 insertions(+) > > diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c > index d5aa121e20..fef4c69004 100644 > --- a/src/conf/virnetworkobj.c > +++ b/src/conf/virnetworkobj.c > @@ -55,6 +55,11 @@ struct _virNetworkObj { > > unsigned int taint; > > + /* fwRemoval contains all commands needed to remove the firewall > + * that was added for this network. > + */ > + virFirewall *fwRemoval; > + > /* Immutable pointer, self locking APIs */ > virMacMap *macmap; > > @@ -239,6 +244,30 @@ virNetworkObjSetFloorSum(virNetworkObj *obj, > } > > > +virFirewall ** > +virNetworkObjGetFwRemovalPtr(virNetworkObj *obj) > +{ > + return &obj->fwRemoval; > +} > + > + > +virFirewall * > +virNetworkObjGetFwRemoval(virNetworkObj *obj) > +{ > + return obj->fwRemoval; > +} > + > + > +void > +virNetworkObjSetFwRemoval(virNetworkObj *obj, > + virFirewall *fwRemoval) > +{ > + obj->fwRemoval = fwRemoval; > + /* give it a name so it's identifiable in the XML */ > + virFirewallSetName(fwRemoval, "fwRemoval"); > +} This call is segv'ing because 'fwRemoval' is NULL if you're starting virnetworkd from an existing host which is currently running old libvirt, as the FW rules won't be in the status XML file. I guess this pristine upgrade scenario is not present on your own machine. Just putting an 'if (fwRemoval)' around the SetName call seems like the right action.

Unless you want to immediately synthesize a virFirewall object with historically accurate rollback rules when loading status XML. Might make later code simpler if you can rely on this being non-NULL ?