[libvirt] [PATCH v6 18/23] conf: support recording ports against virNetworkObjPtr

[Daniel P. Berrangé]

On Mon, Jun 17, 2019 at 03:32:17PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 06, 2019 at 09:51:52PM -0400, Laine Stump wrote:
> > On 5/23/19 11:32 AM, Daniel P. Berrangé wrote:
> > > The virNetworkObjPtr state will need to maintain a record of all
> > > virNetworkPortDefPtr objects associated with the network. Record these
> > > in a hash and add APIs for manipulating them.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > > ---
> > >   src/conf/virnetworkobj.c | 303 +++++++++++++++++++++++++++++++++++++++
> > >   src/conf/virnetworkobj.h |  34 +++++
> > >   src/libvirt_private.syms |   6 +
> > >   3 files changed, 343 insertions(+)
> > > 
> > > diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> > > index c9336e0472..47c142998e 100644
> > > --- a/src/conf/virnetworkobj.c
> > > +++ b/src/conf/virnetworkobj.c
> > > @@ -58,6 +58,8 @@ struct _virNetworkObj {
> > >       /* Immutable pointer, self locking APIs */
> > >       virMacMapPtr macmap;
> > > +
> > > +    virHashTablePtr ports; /* uuid -> virNetworkPortDefPtr */
> > >   };
> > >   struct _virNetworkObjList {
> > > @@ -86,6 +88,17 @@ virNetworkObjOnceInit(void)
> > >   VIR_ONCE_GLOBAL_INIT(virNetworkObj);
> > > +static int
> > > +virNetworkObjLoadAllPorts(virNetworkObjPtr net,
> > > +                          const char *stateDir);
> > > +
> > > +
> > > +static void
> > > +virNetworkObjPortFree(void *val, const void *key ATTRIBUTE_UNUSED)
> > > +{
> > > +    virNetworkPortDefFree(val);
> > > +}
> > > +
> > >   virNetworkObjPtr
> > >   virNetworkObjNew(void)
> > >   {
> > > @@ -106,6 +119,10 @@ virNetworkObjNew(void)
> > >           virBitmapSetBitExpand(obj->classIdMap, 2) < 0)
> > >           goto error;
> > > +    if (!(obj->ports = virHashCreate(10,
> > > +                                     virNetworkObjPortFree)))
> > > +        goto error;
> > > +
> > >       virObjectLock(obj);
> > >       return obj;
> > > @@ -458,6 +475,7 @@ virNetworkObjDispose(void *opaque)
> > >   {
> > >       virNetworkObjPtr obj = opaque;
> > > +    virHashFree(obj->ports);
> > >       virNetworkDefFree(obj->def);
> > >       virNetworkDefFree(obj->newDef);
> > >       virBitmapFree(obj->classIdMap);
> > > @@ -1072,9 +1090,16 @@ virNetworkObjLoadAllState(virNetworkObjListPtr nets,
> > >               continue;
> > >           obj = virNetworkLoadState(nets, stateDir, entry->d_name);
> > > +
> > > +        if (obj &&
> > > +            virNetworkObjLoadAllPorts(obj, stateDir) < 0) {
> > > +            virNetworkObjEndAPI(&obj);
> > > +            goto cleanup;
> > > +        }
> > 
> > 
> > Why do you do this here instead of adding it to virNetworkLoadState()?
> > 
> > 
> > ACK as-is if there's a reason for it. Otherwise, ACK with that chunk moved
> > into virNetworkLoadState()
> 
> Just felt like it would be nice to have them separated, but not a strong
> reason.

Looking at the code again, the virNetworkLoadState() method is already
complicated enough. Moving the port loading into that means adding yet
another cleanup goto label to call the EndAPI method on failure. Clearer
to just do it in the caller

> > Reviewed-by: Laine Stump <laine at laine.org>

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 :|