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.
diff mbox

Message ID 1476299318-1092-1-git-send-email-dbirk@linux.vnet.ibm.com
State Changes Requested
Headers show

Commit Message

Daniel C. Birkestrand Oct. 12, 2016, 7:08 p.m. UTC
From: dbirk <dbirk@us.ibm.com>

---
 configure.ac              | 15 +++++++++++++++
 discover/Makefile.am      | 11 ++++++++++-
 discover/device-handler.c |  7 +++++++
 discover/udev.c           |  2 ++
 4 files changed, 34 insertions(+), 1 deletion(-)

Comments

Stewart Smith Oct. 13, 2016, 5:11 a.m. UTC | #1
"Daniel C. Birkestrand" <dbirk@linux.vnet.ibm.com> writes:
> +		[do not use devmapper, useful for reducing kernel
> dependencies [default=no]]

You'd want to be pretty sure before using this option though, as a "read
only" filesystem mount isn't truly read only, which is why we do the
device mapper stuff in the first place.
Samuel Mendoza-Jonas Oct. 17, 2016, 2:18 a.m. UTC | #2
On Wed, 2016-10-12 at 14:08 -0500, Daniel C. Birkestrand wrote:
> From: dbirk <dbirk@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@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);
>

Patch
diff mbox

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]
+)
+
+AM_CONDITIONAL([HAVE_DEVMAPPER], [test "x$without_devmapper" != "xyes"])
+
+AS_IF([test "x$without_devmapper" = "xyes"],
+	[AC_SUBST([DEFAULT_CPPFLAGS], ["-DDEVMAPPER"])]
+)
+
+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
 
 	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);