...
> > > + 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
