[PATCH] Added --without-devmapper configuration option to eliminate the dependency in the kernel build to help keep overall size small for implementations that have limited space.

[Samuel Mendoza-Jonas]

On Wed, 2016-10-12 at 14:08 -0500, Daniel C. Birkestrand wrote:
> From: dbirk <dbirk at us.ibm.com>

Hi Daniel, thanks for having a look at this!
A few notes below, but firstly the patch will need your signed-off-by line for
me to accept it. Ideally your "name <email>" should include your full name as
well, eg "Daniel C. Birkestrand <dbirk at us.ibm.com>".

Also could you please split up the title of this patch into a small subject and
a comment if needed? The current line is well above 80 chars and messes up the
formatting a bit.

> 
> ---
>  configure.ac              | 15 +++++++++++++++
>  discover/Makefile.am      | 11 ++++++++++-
>  discover/device-handler.c |  7 +++++++
>  discover/udev.c           |  2 ++
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 41560d1..a08a5cb 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -171,6 +171,21 @@ AS_IF(
>  )
>  
>  AC_ARG_WITH(
> +	[devmapper],
> +	[AS_HELP_STRING([--without-devmapper],
> +		[do not use devmapper, useful for reducing kernel dependencies [default=no]]
> +	)],
> +	[without_devmapper=yes],
> +	[without_devmapper=no]
> +)

It would be a good idea to combine this with the existing check for
libdevmapper further above and only perform the check if --without-devmapper
has not been set.

> +
> +AM_CONDITIONAL([HAVE_DEVMAPPER], [test "x$without_devmapper" != "xyes"])
> +
> +AS_IF([test "x$without_devmapper" = "xyes"],
> +	[AC_SUBST([DEFAULT_CPPFLAGS], ["-DDEVMAPPER"])]
> +)

This should define DEVMAPPER but below..

> +
> +AC_ARG_WITH(
>  	[signed-boot],
>  	[AS_HELP_STRING([--with-signed-boot],
>  		[build kernel signature checking support [default=no]]
> diff --git a/discover/Makefile.am b/discover/Makefile.am
> index 4a6cbd0..0a4ef01 100644
> --- a/discover/Makefile.am
> +++ b/discover/Makefile.am
> @@ -24,7 +24,6 @@ discover_pb_discover_SOURCES = \
>  	discover/device-handler.h \
>  	discover/discover-server.c \
>  	discover/discover-server.h \
> -	discover/devmapper.c \
>  	discover/devmapper.h \
>  	discover/event.c \
>  	discover/event.h \
> @@ -54,6 +53,11 @@ discover_pb_discover_SOURCES = \
>  	discover/yaboot-parser.c \
>  	discover/pxe-parser.c
>  
> +if HAVE_DEVMAPPER
> +discover_pb_discover_SOURCES += \
> +	discover/devmapper.c
> +endif
> +
>  discover_pb_discover_LDADD = \
>  	discover/grub2/grub2-parser.ro \
>  	discover/platform.ro \
> @@ -65,6 +69,11 @@ discover_pb_discover_LDFLAGS = \
>  	$(AM_LDFLAGS) \
>  	$(DEVMAPPER_LIBS)
>  
> +if HAVE_DEVMAPPER
> +discover_pb_discover_LDFLAGS += \
> +	-ldevmapper
> +endif
> +
>  discover_pb_discover_CPPFLAGS = \
>  	$(AM_CPPFLAGS) \
>  	-DLOCAL_STATE_DIR='"$(localstatedir)"' \
> diff --git a/discover/device-handler.c b/discover/device-handler.c
> index 346cb02..9f4f6da 100644
> --- a/discover/device-handler.c
> +++ b/discover/device-handler.c
> @@ -1469,7 +1469,9 @@ static int mount_device(struct discover_device *dev)
>  			device_path, strerror(errno));
>  
>  	/* If mount fails clean up any snapshot */
> +#if defined(DEVMAPPER)
>  	devmapper_destroy_snapshot(dev);
> +#endif

..I don't see it defined in these checks in my testing, whether or not
--without-devmapper is specified. You may need to check what is going on with
the configure logic.

These #if defined checks otherwise work but I'd prefer to consolidate them in
one place is possible. For example you could move the #if to devmapper.h and
turn these devmapper_ functions into noops in the negative case.

Cheers,
Sam

>  
>  	pb_rmdir_recursive(mount_base(), dev->mount_path);
>  err_free:
> @@ -1494,7 +1496,10 @@ static int umount_device(struct discover_device *dev)
>  		return -1;
>  
>  	dev->mounted = false;
> +	
> +#if defined(DEVMAPPER)
>  	devmapper_destroy_snapshot(dev);
> +#endif
>  
>  	pb_rmdir_recursive(mount_base(), dev->mount_path);
>  
> @@ -1576,7 +1581,9 @@ void device_release_write(struct discover_device *dev, bool release)
>  	dev->mounted_rw = dev->mounted = false;
>  
>  	if (dev->ramdisk) {
> +#if defined(DEVMAPPER)
>  		devmapper_merge_snapshot(dev);
> +#endif
>  		/* device_path becomes stale after merge */
>  		device_path = get_device_path(dev);
>  	}
> diff --git a/discover/udev.c b/discover/udev.c
> index f4cefab..8f847d6 100644
> --- a/discover/udev.c
> +++ b/discover/udev.c
> @@ -176,10 +176,12 @@ static int udev_handle_block_add(struct pb_udev *udev, struct udev_device *dev,
>  
>  	udev_setup_device_params(dev, ddev);
>  
> +#if defined(DEVMAPPER)
>  	/* Create a snapshot for all disk devices */
>  	if ((ddev->device->type == DEVICE_TYPE_DISK ||
>  	     ddev->device->type == DEVICE_TYPE_USB))
>  		devmapper_init_snapshot(udev->handler, ddev);
> +#endif
>  
>  	device_handler_discover(udev->handler, ddev);
>