[PATCH 2/2] Add flash_lock and flash_unlock applets.

[Denys Vlasenko]

On Friday 22 May 2009 11:08, Thierry Reding wrote:
> Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de>
> ---
>  include/applets.h             |    2 +
>  include/usage.h               |   11 ++++++
>  miscutils/Config.in           |   20 ++++++++++++
>  miscutils/Kbuild              |    1 +
>  miscutils/flash_lock_unlock.c |   70 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 104 insertions(+), 0 deletions(-)
>  create mode 100644 miscutils/flash_lock_unlock.c
> 
> diff --git a/include/applets.h b/include/applets.h
> index 7838757..8ce81a2 100644
> --- a/include/applets.h
> +++ b/include/applets.h
> @@ -155,6 +155,8 @@ IF_FDISK(APPLET(fdisk, _BB_DIR_SBIN, _BB_SUID_NEVER))
>  IF_FEATURE_GREP_FGREP_ALIAS(APPLET_ODDNAME(fgrep, grep, _BB_DIR_BIN, _BB_SUID_NEVER, fgrep))
>  IF_FIND(APPLET_NOEXEC(find, find, _BB_DIR_USR_BIN, _BB_SUID_NEVER, find))
>  IF_FINDFS(APPLET(findfs, _BB_DIR_SBIN, _BB_SUID_MAYBE))
> +IF_FLASH_LOCK(APPLET_ODDNAME(flash_lock, flash_lock_unlock, _BB_DIR_USR_SBIN, _BB_SUID_NEVER, flash_lock))
> +IF_FLASH_UNLOCK(APPLET_ODDNAME(flash_unlock, flash_lock_unlock, _BB_DIR_USR_SBIN, _BB_SUID_NEVER, flash_unlock))
>  //IF_FLASH_ERASEALL(APPLET_ODDNAME(flash_eraseall, flash_eraseall, _BB_DIR_USR_SBIN, _BB_SUID_NEVER, flash_eraseall))
>  IF_FLASH_ERASEALL(APPLET(flash_eraseall, _BB_DIR_USR_SBIN, _BB_SUID_NEVER))

Wrong order. The comment in this file says entries must be
in ascending order.

> +config FLASH_LOCK_UNLOCK
> +	bool
> +	default n
> +
> +config FLASH_LOCK
> +	bool "flash_lock"
> +	select FLASH_LOCK_UNLOCK
> +	default n
> +	help
> +	  The flash_lock binary from mtd-utils as of git head 5ec0c10d0. This
> +	  utility locks part or all of the flash device.
> +
> +config FLASH_UNLOCK
> +	bool "flash_unlock"
> +	select FLASH_LOCK_UNLOCK
> +	default n
> +	help
> +	  The flash_unlock binary from mtd-utils as of git head 5ec0c10d0. This
> +	  utility unlocks part or all of the flash device.
> +

Isn't it simpler to have two options instead of three?

> +	if (strcmp(argv[0], "flash_unlock") == 0)
> +		do_lock = 0;

argv[0] can be "/bin/flash_unlock". Need to look at applet_name.

> +	if (strncmp(argv[1], "/dev/mtd", 8) != 0) {
> +		bb_error_msg_and_die("'%s' is not an MTD device. Must specify "
> +				"an MTD devic: /dev/mtd?\n", argv[1]);
> +	}

Not useful.

> +	fd = xopen(argv[1], O_RDWR);
> +
> +	if (xioctl(fd, MEMGETINFO, &info)) {
> +		bb_error_msg_and_die("failed to get MTD device info from %s\n",
> +				argv[1]);
> +	}

xioctl does not return on error.

> +    xioctl(fd, do_lock ? MEMLOCK : MEMUNLOCK, &lock)

xioctl is a macro. If you use it like that, do_lock ? MEMLOCK : MEMUNLOCK
will get converted to "do_lock ? MEMLOCK : MEMUNLOCK" string
in error message. Not good.

I fixed these problems and added it to git repo. Please test
and let me know if something doesn't work.

Thanks!
--
vda


--
vda