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

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

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

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