[libvirt PATCH v4 09/25] nodedev: add mdevctl devices to node device list

[Erik Skultety]

...

> > > +            bool changed;
> > > +            virNodeDeviceDefPtr olddef =
> > > virNodeDeviceObjGetDef(obj); +
> > > +            was_defined = virNodeDeviceObjIsPersistent(obj);  
> > 
> >     "defined" as a name will do just fine
> > 
> > > +            /* Active devices contain some additional information
> > > (e.g. sysfs
> > > +             * path) that is not provided by mdevctl, so re-use
> > > the existing
> > > +             * definition and copy over new mdev data */
> > > +            changed = nodeDeviceDefCopyFromMdevctl(olddef, def);
> > > +
> > > +            /* Re-use the existing definition (after merging new
> > > data from
> > > +             * mdevctl), so def needs to be freed at the end of
> > > the function */
> > > +            tofree = g_list_prepend(tofree, def);  
> > 
> >     I'm not a fan of ^this. Plain g_autoptr on virNodeDeviceDef would
> > do just fine.
> 
> I'm not a fan of it either, but g_autoptr is not ideal here either.
> The def must not be freed if it is a new device (because 
> virNodeDeviceObjListAssignDef() takes ownership of the def). So we'd
> have to make sure we steal the autoptr in that case. But we have to use
> it after we'd need to clear it (in order to emit the lifecycle event,
> etc).
> 
> But there's also another reason I took this approach. In patch 11
> ("handle mdevs that disappear..."), we need to use the entire list of
> devices ('defs') to scan for devices that have disappeared. So I had
> to keep the entire list of defs until the end of the function where we
> traversed the list looking for removed devices to emit a lifecycle
> event. I could potentially move this check to the beginning of the
> function to get around the need to keep the defs around until the end
> of the function, though...
> 
> I can try to rework it if you think it necessary.

I think purging non-existent devices at the beginning of the function is fine,
maybe even desirable. As for the ownership, you only need def->name for events
so I think it would still be easily doable by dropping the 'tofree' list.

...

> > > +        if (STRNEQ_NULLABLE(src->attributes[i]->value,
> > > +                            dst->attributes[i]->value)) {
> > > +            ret = true;
> > > +            dst->attributes[i]->value =
> > > +                g_strdup(src->attributes[i]->value);
> > > +        }
> > > +    }  
> > 
> > I think we can just straight overwrite all the data in question, yes,
> > some CPU cycles will be wasted, but time complexity remains at O(n)
> > and readability improves significantly. These are mdev devices, so I
> > doubt this is an RT critical feature. The function can then remain as
> > void.
> 
> It's perhaps not obvious, but I specifically added these checks and
> return value in order to avoid an issue that Daniel pointed out where I
> was emitting update events for every device regardless of whether they
> had changed or not. So rather than inventing a new function to check for
> node device equality and only copy over the new data if they weren't
> equal, I incorporated the equality check into the update function so
> that I could check it and update it in the same call. See above where I
> use the 'changed' variable in nodeDeviceUpdateMediatedDevices().

Darn...yeah, I missed that, it's fine as it is then.

...

> > > +        if (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV)
> > > +            nodeDeviceDefCopyFromMdevctl(def, objdef);
> > > +        was_persistent = virNodeDeviceObjIsPersistent(obj);
> > > +        /* If the device was defined by mdevctl and was never
> > > instantiated, it
> > > +         * won't have a sysfs path. We need to emit a CREATED
> > > event... */  
> > 
> >     Why CREATED instead of DEFINED?
> 
> Because this is handling a udev event. udev only deals with with
> instantiated mdevs, not persistent mdev definitions.
> 
> In other words, mdevctl determines whether a mdev is DEFINED or
> UNDEFINED, and udev determines whether a devices is CREATED or DELETED.

Okay, thanks for refreshing my memory.

Erik