Re: two patches

[Dan Nicholson <dbn.lists@gmail.com>]

On Fri, Apr 16, 2010 at 09:50:42AM +0400, Yury G. Kudryashov wrote:
> Hi!
> 
> Review the attached patches, please.

> From 5cc42c6f87f690fdf66e29de2a816dab49a119f4 Mon Sep 17 00:00:00 2001
> From: Yury G. Kudryashov <urkud.urkud@gmail.com>
> Date: Fri, 16 Apr 2010 00:21:02 +0400
> Subject: [PATCH 1/2] Include linux/types.h

1. Prefix the subject with "hid2hci: " so it's clear from the shortlog what
   this affects.

2. Please include some rational in the commit message. What prototype or
   datatype is missing that requires including <linux/types.h>?

> 
> ---
>  extras/hid2hci/hid2hci.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/extras/hid2hci/hid2hci.c b/extras/hid2hci/hid2hci.c
> index 0d0a022..839c4fb 100644
> --- a/extras/hid2hci/hid2hci.c
> +++ b/extras/hid2hci/hid2hci.c
> @@ -28,6 +28,7 @@
>  #include <string.h>
>  #include <getopt.h>
>  #include <sys/ioctl.h>
> +#include <linux/types.h>
>  #include <linux/hiddev.h>
>  #include <usb.h>
>  
> -- 
> 1.7.0.5
> 
> 

> From f9881e554deafad1301d00e4539a02fe63078c00 Mon Sep 17 00:00:00 2001
> From: Yury G. Kudryashov <urkud.urkud@gmail.com>
> Date: Fri, 16 Apr 2010 09:38:32 +0400
> Subject: [PATCH 2/2] Add --with-firmware-path configure option

Again, please include some rational in the commit message. What problem is
this solving?

> 
> ---
>  Makefile.am                |    3 ++-
>  configure.ac               |   22 ++++++++++++++++++++++
>  extras/firmware/firmware.c |   10 ++++++----
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 68a68d9..f094746 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -156,7 +156,7 @@ EXTRA_DIST += \
>  	udev/udevd.xml
>  
>  %.7 %.8 : %.xml
> -	$(XSLTPROC) -o $@ -nonet http://docbook.sourceforge.net/release/xsl/current/manpages/docbook.xsl $<
> +	$(XSLTPROC) -o $@ http://docbook.sourceforge.net/release/xsl/current/manpages/docbook.xsl $<

This is an unrelated change. Please remove it from this patch.

>  
>  # ------------------------------------------------------------------------------
>  # udev tests
> @@ -194,6 +194,7 @@ dist_udevrules_DATA += \
>  # ------------------------------------------------------------------------------
>  extras_firmware_firmware_SOURCES = extras/firmware/firmware.c
>  extras_firmware_firmware_LDADD = libudev/libudev-private.la
> +extras_firmware_firmware_CPPFLAGS = $(AM_CPPFLAGS) -DFIRMWARE_PATH="$(FIRMWARE_PATH)"
>  dist_udevrules_DATA += extras/firmware/50-firmware.rules
>  libexec_PROGRAMS = extras/firmware/firmware
>  
> diff --git a/configure.ac b/configure.ac
> index 492fa02..5defd5c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -82,6 +82,27 @@ if test "x$enable_extras" = xyes; then
>  		fi])
>  	AC_SUBST(PCI_DATABASE)
>  
> +	AC_ARG_WITH(firmware-path,
> +		AS_HELP_STRING([--with-firmware-path=DIR[[[:DIR[...]]]]],
> +		   [Firmware search path (default=/lib/firmware/updates:/lib/firmware)]),
> +		[],
> +		[with_firmware_path="/lib/fimware/updates:/lib/fimware"]
> +		)
> +	[
> +	OLD_IFS=$IFS
> +	IFS=:
> +	for i in $with_firmware_path
> +	do
> +		if [ "x${FIRMWARE_PATH}" = "x" ]; then
> +			FIRMWARE_PATH="\\\"${i}/\\\""
> +		else
> +			FIRMWARE_PATH="${FIRMWARE_PATH}, \\\"${i}/\\\""
> +		fi
> +	done
> +	IFS=$OLD_IFS
> +	]

I'm not sure what wrapping this whole section in [] quoting does. It looks
like a good handling of the option, though.

> +	AC_SUBST([FIRMWARE_PATH], [$FIRMWARE_PATH])
> +
>  	AC_CHECK_HEADER([linux/input.h], [:], AC_MSG_ERROR([kernel headers not found]))
>  	AC_SUBST([INCLUDE_PREFIX], [$(echo '#include <linux/input.h>' | eval $ac_cpp -E - | sed -n '/linux\/input.h/ {s:.*"\(.*\)/linux/input.h".*:\1:; p; q}')])
>  fi
> @@ -144,6 +165,7 @@ AC_MSG_RESULT([
>  
>  	usb.ids:		${USB_DATABASE}
>  	pci.ids:		${PCI_DATABASE}
> +	firmware path:		${FIRMWARE_PATH}
>  
>  	xsltproc:		${XSLTPROC}
>  	gperf:			${GPERF}
> diff --git a/extras/firmware/firmware.c b/extras/firmware/firmware.c
> index 92f0918..76071db 100644
> --- a/extras/firmware/firmware.c
> +++ b/extras/firmware/firmware.c
> @@ -79,10 +79,7 @@ int main(int argc, char **argv)
>  		{ "help", no_argument, NULL, 'h' },
>  		{}
>  	};
> -	static const char *searchpath[] = {
> -		"/lib/firmware/updates/",
> -		"/lib/firmware/"
> -	};
> +	static const char *searchpath[] = { FIRMWARE_PATH };
>  	char fwencpath[UTIL_PATH_SIZE];
>  	char misspath[UTIL_PATH_SIZE];
>  	char loadpath[UTIL_PATH_SIZE];
> @@ -97,6 +94,11 @@ int main(int argc, char **argv)
>  	unsigned int i;
>  	int rc = 0;
>  
> +	for (i = 0; i < ARRAY_SIZE(searchpath); i++) {
> +		printf("Path %s\n", searchpath[i]);
> +	}
> +	return 0;
> +

I'm pretty sure the program stops doing anything useful with this hunk. :)

>  	udev_log_init("firmware");
>  
>  	for (;;) {
> -- 
> 1.7.0.5

Besides those small issues, both patches seem pretty good to me.

--
Dan