Re: [RFC PATCH 15/20] libmultipath: API for foreign multipath handling

["Benjamin Marzinski" <bmarzins@redhat.com>]

On Tue, Feb 20, 2018 at 02:26:53PM +0100, Martin Wilck wrote:
> Add an API for "foreign" multipaths. Foreign libraries are loaded
> from ${multipath_dir}/libforeign-*.so, as we do for checkers.
> 
> Refer to "foreign.h" for details about the API itself. Like we do for
> checkers, high-level multipath code isn't supposed to call the API directly,
> but rather the wrapper functions declared in "foreign.h".
> 
> This API is used only for displaying information and for logging. An extension to
> other functionality (such as monitoring or administration) might be feasible,
> but is not planned.
> 
> Foreign libraries communicate with libmultipath through the API defined in
> "foreign.h". The foreign library can implement multipath maps, pathgroups,
> and paths as it likes, they just need to provide the simple interfaces
> defined in "generic.h" to libmultipath. These interfaces are used in libmultipath's
> "print" implementation to convey various bits of information to users. By
> using the same interfaces for printing that libmultipath uses internally,
> foreign library implementations can focus on the technical side without
> worrying about output formatting compatibility.

ACK, with one minor nit

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/Makefile  |   2 +-
>  libmultipath/foreign.c | 471 +++++++++++++++++++++++++++++++++++++++++++++++++
>  libmultipath/foreign.h | 322 +++++++++++++++++++++++++++++++++
>  3 files changed, 794 insertions(+), 1 deletion(-)
>  create mode 100644 libmultipath/foreign.c
>  create mode 100644 libmultipath/foreign.h

<snip>

> +struct foreign {
> +	/**
> +	 * method: init(api, name)
> +	 * Initialize foreign library, and check API compatibility
> +	 * return pointer to opaque internal data strucure if successful,
> +	 * NULL otherwise.
> +	 *
> +	 * @param[in] api: API version
> +	 * @param[in] name: name to use for references to self in log messages,
> +	 *     doesn't need to be strdup'd
> +	 * @returns context pointer to use in future method calls.
> +	 */
> +	struct context* (*init)(unsigned int api, const char *name);
> +
> +	/**
> +	 * method: cleanup(context)
> +	 * Free data structures used by foreign library, including
> +	 * context itself.
> +	 *
> +	 * @param[in] context foreign library context. This shouldn't be
> +	 * referenced any more after calling cleanup().
> +	 */
> +	void (*cleanup)(struct context *);
> +
> +	/**
> +	 * method: add(context, udev)
> +	 * This is called during path detection, and for udev ADD events.
> +	 *
> +	 * @param[in] context foreign library context
> +	 * @param[in] udev udev device to add
> +	 * @returns status code
> +	 * @retval FOREIGN_CLAIMED: device newly claimed
> +	 * @retval FOREIGN_OK: device already registered, no action taken
> +	 * @retval FOREIGN_IGNORED: device is ignored, no action taken
> +	 * @retval FOREIGN_ERR: an error occured (e.g. out-of-memory)
> +	 */
> +	int (*add)(struct context *, struct udev_device *);
> +
> +	/**
> +	 * method: change
> +	 * This is called on udev CHANGE events.
> +	 *
> +	 * @param[in] context foreign library context
> +	 * @param[in] udev udev device that has generated the event
> +	 * @returns status code
> +	 * @retval FOREIGN_OK: event processed
> +	 * @retval FOREIGN_IGNORED: the device is ignored
> +	 * @retval FOREIGN_ERR: an error occured (e.g. out-of-memory)
> +	 *
> +	 * Note: theoretically it can happen that the status of a foreign device
> +	 * (claimed vs. not claimed) changes in a change event.
> +	 * Supporting this correctly would require big efforts. For now, we
> +	 * don't support it. "multipathd reconfigure" starts foreign device
> +	 * detection from scratch and should be able to handle this situation.
> +	 */
> +	int (*change)(struct context *, struct udev_device *);
> +
> +	/**
> +	 * method: delete
> +	 * This is called on udev DELETE events.
> +	 *
> +	 * @param[in] context foreign library context
> +	 * @param[in] udev udev device that has generated the event and
> +	 *	should be deleted
> +	 * @returns status code
> +	 * @retval FOREIGN_OK: processed correctly (device deleted)
> +	 * @retval FOREIGN_IGNORED: device wasn't registered internally
> +	 * @retval FOREIGN_ERR: error occured.
> +	 */
> +	int (*delete)(struct context *, struct udev_device *);
> +
> +	/**
> +	 * method: delete_all
> +	 * This is called if multipathd reconfigures itself.
> +	 * Deletes all registered devices (maps and paths)
> +	 *
> +	 * @param[in] context foreign library context
> +	 * @returns status code
> +	 * @retval FOREIGN_OK: processed correctly
> +	 * @retval FOREIGN_IGNORED: nothing to delete
> +	 * @retval FOREIGN_ERR: error occured
> +	 */
> +	int (*delete_all)(struct context*);
> +
> +	/**
> +	 * method: check
> +	 * This is called from multipathd's checker loop.
> +	 *
> +	 * Check status of managed devices, update internal status, and print
> +	 * log messages if appropriate.
> +	 * @param[in] context foreign library context
> +	 */
> +	void (*check)(struct context *);
> +
> +	/**
> +	 * lock internal data stuctures.
> +	 * @param[in] ctx: foreign context
> +	 */
> +	void (*lock)(struct context *ctx);
> +
> +	/**
> +	 * unlock internal data stuctures.
> +	 * @param[in] ctx: foreign context (void* in order to use the function
> +	 *	as argument to pthread_cleanup_push())
> +	 */
> +	void (*unlock)(void *ctx);
> +
> +	/**
> +	 * method: get_multipaths(context)
> +	 * Returned vector must be freed by calling release_multipaths().
> +	 * Lock must be held until release_multipaths() is called.
> +	 *
> +	 * @param[in] context foreign library context
> +	 * @returns a vector of "struct gen_multipath*" with the map devices
> +	 * belonging to this library (see generic.h).
> +	 */
> +	const struct _vector* (*get_multipaths)(const struct context *);
> +
> +	/**
> +	 * method: release_multipaths(context, mpvec)
> +	 * release data structures obtained with get_multipaths (if any)
> +	 *
> +	 * @param[in] ctx the foreign context
> +	 * @param[in] mpvec the vector allocated with get_multipaths()
> +	 */
> +	void (*release_multipaths)(const struct context *ctx,
> +				   const struct _vector* mpvec);
> +
> +	/**
> +	 * method: get_paths
> +	 * Returned vector must be freed by calling release_paths().
> +	 * Lock must be held until release_paths() is called.
> +	 *
> +	 * @param[in] context foreign library context
> +	 * @returns a vector of "struct gen_path*" with the path devices
> +	 * belonging to this library (see generic.h)
> +	 */
> +	const struct _vector* (*get_paths)(const struct context *);
> +
> +	/**
> +	 * release data structures obtained with get_multipaths (if any)
> +	 *
> +	 * @param[in] ctx the foreign context
> +	 * @param[in] ppvec the vector allocated with get_paths()
> +	 */
> +	void (*release_paths)(const struct context *ctx,
> +			      const struct _vector* ppvec);
> +
> +	const char *name;
> +	void *handle;
> +	struct context *context;
> +};

Instead of having a separate pointer "name", would you mind using
const char name[0];

at the end of the structure, like struct logmsg does? I believe this
should work with const.

-Ben

> +
> +/**
> + * init_foreign(dir)
> + * load and initialize foreign multipath libraries in dir (libforeign-*.so).
> + * @param dir: directory to search
> + * @returns: 0 on success, negative value on failure.
> + */
> +int init_foreign(const char *multipath_dir);
> +
> +/**
> + * cleanup_foreign(dir)
> + * cleanup and free all data structures owned by foreign libraries
> + */
> +void cleanup_foreign(void);
> +
> +/**
> + * add_foreign(udev)
> + * check if a device belongs to any foreign library.
> + * calls add() for all known foreign libs, in the order registered,
> + * until the first one returns FOREIGN_CLAIMED or FOREIGN_OK.
> + * @param udev: udev device to check
> + * @returns: status code
> + * @retval FOREIGN_CLAIMED: newly claimed by a foreign lib
> + * @retval FOREIGN_OK: already claimed by a foreign lib
> + * @retval FOREIGN_IGNORED: ignored by all foreign libs
> + * @retval FOREIGN_ERR: an error occured
> + */
> +int add_foreign(struct udev_device *);
> +
> +/**
> + * change_foreign(udev)
> + * Notify foreign libraries of an udev CHANGE event
> + * @param udev: udev device to check
> + * @returns: status code (see change() method above).
> + */
> +int change_foreign(struct udev_device *);
> +
> +/**
> + * delete_foreign(udev)
> + * @param udev: udev device being removed
> + * @returns: status code (see remove() above)
> + */
> +int delete_foreign(struct udev_device *);
> +
> +/**
> + * delete_all_foreign()
> + * call delete_all() for all foreign libraries
> + * @returns: status code (see delete_all() above)
> + */
> +int delete_all_foreign(void);
> +
> +/**
> + * check_foreign()
> + * call check() (see above) for all foreign libraries
> + */
> +void check_foreign(void);
> +
> +/**
> + * foreign_path_layout()
> + * call this before printing paths, after get_path_layout(), to determine
> + * output field width.
> + */
> +void foreign_path_layout(void);
> +
> +/**
> + * foreign_multipath_layout()
> + * call this before printing maps, after get_multipath_layout(), to determine
> + * output field width.
> + */
> +void foreign_multipath_layout(void);
> +
> +/**
> + * snprint_foreign_topology(buf, len, verbosity);
> + * prints topology information from foreign libraries into buffer,
> + * '\0' - terminated.
> + * @param buf: output buffer
> + * @param len: size of output buffer
> + * @param verbosity: verbosity level
> + * @returns: number of printed characters excluding trailing '\0'.
> + */
> +int snprint_foreign_topology(char *buf, int len, int verbosity);
> +
> +/**
> + * snprint_foreign_paths(buf, len, style, pad);
> + * prints formatted path information from foreign libraries into buffer,
> + * '\0' - terminated.
> + * @param buf: output buffer
> + * @param len: size of output buffer
> + * @param style: format string
> + * @param pad: whether to pad field width
> + * @returns: number of printed characters excluding trailing '\0'.
> + */
> +int snprint_foreign_paths(char *buf, int len, const char *style, int pad);
> +
> +/**
> + * snprint_foreign_multipaths(buf, len, style, pad);
> + * prints formatted map information from foreign libraries into buffer,
> + * '\0' - terminated.
> + * @param buf: output buffer
> + * @param len: size of output buffer
> + * @param style: format string
> + * @param pad: whether to pad field width
> + * @returns: number of printed characters excluding trailing '\0'.
> + */
> +int snprint_foreign_multipaths(char *buf, int len,
> +			       const char *style, int pretty);
> +
> +/**
> + * print_foreign_topology(v)
> + * print foreign topology to stdout
> + * @param verbosity: verbosity level
> + */
> +void print_foreign_topology(int verbosity);
> +
> +/**
> + * is_claimed_by_foreign(ud)
> + * @param udev: udev device
> + * @returns: true iff device is (newly or already) claimed by a foreign lib
> + */
> +static inline bool
> +is_claimed_by_foreign(struct udev_device *ud)
> +{
> +	int rc = add_foreign(ud);
> +
> +	return (rc == FOREIGN_CLAIMED || rc == FOREIGN_OK);
> +}
> +
> +#endif /*  _FOREIGN_H */
> -- 
> 2.16.1