Message ID | 1476299318-1092-1-git-send-email-dbirk@linux.vnet.ibm.com |
---|---|
State | Changes Requested |
Headers | show |
"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.
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); >
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);
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(-)