Re: [RFC PATCH 17/20] libmultipath/foreign: nvme foreign library

[Martin Wilck <mwilck@suse.com>]

On Wed, 2018-02-28 at 21:14 -0600, Benjamin Marzinski wrote:
> On Tue, Feb 20, 2018 at 02:26:55PM +0100, Martin Wilck wrote:
> > This still contains stubs for path handling and checking, but it's
> > functional
> > for printing already.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  Makefile                      |   1 +
> >  libmultipath/foreign/Makefile |  30 +++
> >  libmultipath/foreign/nvme.c   | 444
> > ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 475 insertions(+)
> >  create mode 100644 libmultipath/foreign/Makefile
> >  create mode 100644 libmultipath/foreign/nvme.c
> > 
> > diff --git a/libmultipath/foreign/nvme.c
> > b/libmultipath/foreign/nvme.c
> > new file mode 100644
> > index 000000000000..4e9c3a52d03c
> > --- /dev/null
> > +++ b/libmultipath/foreign/nvme.c
> > 
> > +static void cleanup_nvme_map(struct nvme_map *map)
> > +{
> > +	if (map->udev)
> > +		udev_device_unref(map->udev);
> > +	if (map->subsys)
> > +		udev_device_unref(map->subsys);
> 
> According to the libudev reference manual, udev devices returned by
> udev_device_get_parent_with_subsystem_devtype are attached to their
> child and free along with them, so this unref shouldn't be necessary

I'd figured that myself and fixed it in patch 20/20. It must have
excaped my attention when I cleaned up. I'll fix the sequence.

> +
> > +void cleanup(struct context *ctx)
> > +{
> > +	if (ctx == NULL)
> > +		return;
> > +
> > +	(void)delete_all(ctx);
> > +
> > +	lock(ctx);
> > +	pthread_cleanup_push(unlock, ctx);
> > +	vector_free(ctx->mpvec);
> > +	pthread_cleanup_pop(1);
> > +	pthread_mutex_destroy(&ctx->mutex);
> > +
> > +	free(ctx);
> > +}
> 
> Would you mind either removing the locking, or adding a comment
> explaining that it's not necessary here.  If you really did need to
> lock
> ctx in order guard against someone accessing ctx->mpvec in cleanup(),
> then not setting it to NULL after its freed, and immediately freeing
> ctx
> afterwards would clearly be broken, so seeing the locking makes it
> look
> like this function is wrong.

I don't understand, sorry. I need to lock because some other entity may
still be using ctx->mpvec when cleanup() is called, and I should wait
until that other entity has released the lock before I free it. I'm
also pretty sure that checkers such as coverity would complain if I
free a structure here without locking which I access only under the
lock in other places.

I agree, though, that I should nullify the data and add checks in the
other functions. I'll also add some locking in the foreign.c file,
didn't occur to me before.

> > +static int _add_map(struct context *ctx, struct udev_device *ud,
> > +		    struct udev_device *subsys)
> > +{
> > +	dev_t devt = udev_device_get_devnum(ud);
> > +	struct nvme_map *map;
> > +
> > +	if (_find_nvme_map_by_devt(ctx, devt) != NULL)
> > +		return FOREIGN_OK;
> > +
> > +	map = calloc(1, sizeof(*map));
> > +	if (map == NULL)
> > +		return FOREIGN_ERR;
> > +
> > +	map->devt = devt;
> > +	map->udev = udev_device_ref(ud);
> > +	map->subsys = udev_device_ref(subsys);
> 
> You shouldn't need to get a reference here, since map->udev will keep
> this device around.

See above.

Regards,
Martin

> 
> -Ben
> 
> > +	map->gen.ops = &nvme_map_ops;
> > +
> > +	if (vector_alloc_slot(ctx->mpvec) == NULL) {
> > +		cleanup_nvme_map(map);
> > +		return FOREIGN_ERR;
> > +	}
> > +
> > +	vector_set_slot(ctx->mpvec, map);
> > +
> > +	return FOREIGN_CLAIMED;
> > +}
> > +
> > +int add(struct context *ctx, struct udev_device *ud)
> > +{
> > +	struct udev_device *subsys;
> > +	int rc;
> > +
> > +	condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +
> > +	if (ud == NULL)
> > +		return FOREIGN_ERR;
> > +	if (strcmp("disk", udev_device_get_devtype(ud)))
> > +		return FOREIGN_IGNORED;
> > +
> > +	subsys = udev_device_get_parent_with_subsystem_devtype(ud,
> > +							       "nv
> > me-subsystem",
> > +							       NUL
> > L);
> > +	if (subsys == NULL)
> > +		return FOREIGN_IGNORED;
> > +
> > +	lock(ctx);
> > +	pthread_cleanup_push(unlock, ctx);
> > +	rc = _add_map(ctx, ud, subsys);
> > +	pthread_cleanup_pop(1);
> > +
> > +	if (rc == FOREIGN_CLAIMED)
> > +		condlog(3, "%s: %s: added map %s", __func__, THIS,
> > +			udev_device_get_sysname(ud));
> > +	else if (rc != FOREIGN_OK)
> > +		condlog(1, "%s: %s: retcode %d adding %s",
> > +			__func__, THIS, rc,
> > udev_device_get_sysname(ud));
> > +
> > +	return rc;
> > +}
> > +
> > +int change(struct context *ctx, struct udev_device *ud)
> > +{
> > +	condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +	return FOREIGN_IGNORED;
> > +}
> > +
> > +static int _delete_map(struct context *ctx, struct udev_device
> > *ud)
> > +{
> > +	int k;
> > +	struct nvme_map *map;
> > +	dev_t devt = udev_device_get_devnum(ud);
> > +
> > +	map = _find_nvme_map_by_devt(ctx, devt);
> > +	if (map ==NULL)
> > +		return FOREIGN_IGNORED;
> > +
> > +	k = find_slot(ctx->mpvec, map);
> > +	if (k == -1)
> > +		return FOREIGN_ERR;
> > +	else
> > +		vector_del_slot(ctx->mpvec, k);
> > +
> > +	cleanup_nvme_map(map);
> > +
> > +	return FOREIGN_OK;
> > +}
> > +
> > +int delete(struct context *ctx, struct udev_device *ud)
> > +{
> > +	int rc;
> > +
> > +	condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +
> > +	if (ud == NULL)
> > +		return FOREIGN_ERR;
> > +
> > +	lock(ctx);
> > +	pthread_cleanup_push(unlock, ctx);
> > +	rc = _delete_map(ctx, ud);
> > +	pthread_cleanup_pop(1);
> > +
> > +	if (rc == FOREIGN_OK)
> > +		condlog(3, "%s: %s: map %s deleted", __func__,
> > THIS,
> > +			udev_device_get_sysname(ud));
> > +	else if (rc != FOREIGN_IGNORED)
> > +		condlog(1, "%s: %s: retcode %d deleting map %s",
> > __func__,
> > +			THIS, rc, udev_device_get_sysname(ud));
> > +
> > +	return rc;
> > +}
> > +
> > +void check(struct context *ctx)
> > +{
> > +	condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +	return;
> > +}
> > +
> > +/*
> > + * It's safe to pass our internal pointer, this is only used under
> > the lock.
> > + */
> > +const struct _vector *get_multipaths(const struct context *ctx)
> > +{
> > +	condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +	return ctx->mpvec;
> > +}
> > +
> > +void release_multipaths(const struct context *ctx, const struct
> > _vector *mpvec)
> > +{
> > +	condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +	/* NOP */
> > +}
> > +
> > +/*
> > + * It's safe to pass our internal pointer, this is only used under
> > the lock.
> > + */
> > +const struct _vector * get_paths(const struct context *ctx)
> > +{
> > +	condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +	return NULL;
> > +}
> > +
> > +void release_paths(const struct context *ctx, const struct _vector
> > *mpvec)
> > +{
> > +	condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +	/* NOP */
> > +}
> > +
> > +/* compile-time check whether all methods are present and
> > correctly typed */
> > +#define _METHOD_INIT(x) .x = x
> > +static struct foreign __methods __attribute__((unused)) = {
> > +	_METHOD_INIT(init),
> > +	_METHOD_INIT(cleanup),
> > +	_METHOD_INIT(change),
> > +	_METHOD_INIT(delete),
> > +	_METHOD_INIT(delete_all),
> > +	_METHOD_INIT(check),
> > +	_METHOD_INIT(lock),
> > +	_METHOD_INIT(unlock),
> > +	_METHOD_INIT(get_multipaths),
> > +	_METHOD_INIT(release_multipaths),
> > +	_METHOD_INIT(get_paths),
> > +	_METHOD_INIT(release_paths),
> > +};
> > -- 
> > 2.16.1
> 
> 

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel