[CRIU] [PATCH 2/3] plugin: Rework plugins API

[Andrew Vagin]

On Wed, Mar 12, 2014 at 03:05:06PM +0400, Cyrill Gorcunov wrote:
> On Wed, Mar 12, 2014 at 02:57:27PM +0400, Andrew Vagin wrote:
> > > +
> > > +/*
> > > + * Strictly speaking this CR_PRIV_CHAIN_ENTRY_SIZE stands for
> > > + * double linked list entry size.
> > > + */
> > > +#define CR_PRIV_CHAIN_ENTRY_SIZE	(2 * sizeof(void *))
> > > +#define CR_PRIV_SIZE			((CR_PLUGIN_HOOK__MAX) * CR_PRIV_CHAIN_ENTRY_SIZE + \
> > > +					 CR_PRIV_CHAIN_ENTRY_SIZE + 16 * sizeof(void *))
> > 
> > What is a magic 16 here?
> 
> 16 cells for further use if ever needed.
> 
> > > +typedef struct {
> > > +	const char		*name;
> > > +	int			(*init)(void);
> > > +	void			(*exit)(void);
> > > +	unsigned int		version;
> > > +	unsigned int		max_hooks;

struct {
	int (*func)();
	cr_plugin_desc_t *next; /* Don't touch this field from plugins */
} hooks[CR_PLUGIN_HOOK__MAX];

Can we use this code and remove the private blob?

> > > +	void			*hooks[CR_PLUGIN_HOOK__MAX];
> > > +	size_t			priv_size;
> > > +
> > > +	/*
> > > +	 * @private is CRIU plugin engine private field,
> > > +	 * don't ever modify inside plugin code!
> > > +	 *
> > > +	 * Also if this structure need to be extended
> > > +	 * put new members _AFTER_ the @private field.
> > > +	 */
> > > +	char			private[CR_PRIV_SIZE];
> > 
> > How we are going to support its backward compatibility in a future?
> 
> By version + max_hooks tuple. If we ever change API so that
> new format won;t be compatible with new one -- we always
> can detect it by comparing @version and wrap data with
> some new descriptor generated in memory.
> 
> > 
> > > +} __attribute__((__packed__)) cr_plugin_desc_t;
> > 
> > Why do we need __packed__ here?
> 
> To make sure that there is no holes or anything in the
> structure. I must admit it's not that required but
> decided to put it here just to be safe.
> 
> > > +extern cr_plugin_desc_t CR_PLUGIN_DESC;
> > > +
> > > +#define CR_PLUGIN_REGISTER(___name, ___init, ___exit)					\
> > > +	cr_plugin_desc_t CR_PLUGIN_DESC = {						\
> > > +		.name		= ___name,						\
> > > +		.init		= ___init,						\
> > > +		.exit		= ___exit,						\
> > > +		.version	= CRIU_PLUGIN_VERSION,					\
> > > +		.max_hooks	= CR_PLUGIN_HOOK__MAX,					\
> > > +		.priv_size	= CR_PRIV_SIZE,						\
> > > +	}
> > > +
> > > +static inline int cr_plugin_dummy_init(void) { return 0; }
> > > +static inline void cr_plugin_dummy_exit(void) { }
> > 
> > I think inline is useless here, because you are going to access these
> > function by pointers.
> 
> Without inline I've got compile warnings in earlier development stages
> of this code. Not a big deal can drop it out.