Hi James,
On Mon, 09 Mar 2009 09:37:58 -0400 James Carlson <James.D.Carlson at Sun.COM> wrote: > Alan Hargreaves writes: > > and if an identification plugin defines > > > > unsigned int plugin_version = FWPLUGIN_VERSION_2; > > > > in its struct fw_plugin definition, then the fwflash cleanup function > > I don't think I understand that. First of all (as a nit), 2008/151 > didn't include versioning, so it'd be good to describe how it works > here. I didn't worry about versioning with 2008/151 - and I should have thought further when designing it. Benefit of hindsight :| The idea is that when a new structure element (for any of the existing structures) or a new structure is defined, we would bump the version number, so $SRC/cmd/fwflash/common/fwflash.c would be able to deal with the change in an appropriate fashion. Existing plugins would not need to be redelivered because their behaviour would not change. > Reading the existing header file, it seems that plugins must define a > uint32_t variable (not a struct) that has the name "fwplugin_version", > and that's what's used for the versioning, not "plugin_version" as > above. > Is that correct, or are there other changes here? Does the integer > change to a structure? Is there a new variable? Reviewing the existing header file, I see that there are extraneous comments about versioning left over from a very early version of the original spec. The existing comments are wrong, not only are plugins not required to define any version-related element in any structure, but even if they did (now), we have no way of using that information. We are proposing to add the integer plugin_version (or fwplugin_version, I'm not wedded to the exact name). There is no new structure. > I would have expected just a new #define. > > > int (*fw_cleanup)(struct devicelist *thisdev); > > > > It is an error condition for an identification plugin to define > > plugin_version >= FWPLUGIN_VERSION_2, and to not also define the > > fw_cleanup() function. > > One tiny nit here: I think you'd probably be better off making it so > that plugins simply use FWPLUGIN_VERSION_CURRENT, and the framework > calls fw_cleanup if the version is >=2 and the function pointer is > non-NULL. > That way, old plugins will correctly inform the framework that they've > been compiled with an old version of this structure (and thus the new > fw_cleanup member is located one word off the end) by saying version > 1, and new plugins that don't need to clean anything up won't be > forced into providing a dummy function. For the teardown part, that's pretty much how I've got it implemented (except for the #define name) in my prototype. > (The real issue here is that structures like this should be allocated > by the framework so that you don't have to bump a version number when > new members are allocated. The versioning needs to be tied with the > allocation of space for that structure. But I guess it's too late to > fix that ...) Yes, I agree. Again, benefit of hindsight. For that matter, if I was designing Pluggable fwflash all over again I'd skip use of the <sys/queue.h> header and just use libnvpair instead. James C. McPherson -- Senior Kernel Software Engineer, Solaris Sun Microsystems http://blogs.sun.com/jmcp http://www.jmcp.homeunix.com/blog
