On Tue, Mar 16, 2010 at 1:14 PM, Timo Teras <[email protected]> wrote:
> An utility to manage file locks from scripts.
>
> Signed-off-by: Timo Teras <[email protected]>
Does it
> +#define flock_trivial_usage \
> + "[-sxun] [fd# | FILE] [-c] command"
Follow the style of other help texts:
s/command/PROG ARGS/
this helps help text to compress better.
> +#include <sys/types.h>
> +#include <sys/file.h>
> +#include <sys/stat.h>
> +#include <signal.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include "busybox.h"
Just one #include "libbb.h" will do.
> +int flock_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
> +int flock_main(int argc, char **argv)
int argc UNUSED_PARAM
> + opt = getopt32(argv, "sxunc:", &command);
> + argv += optind;
Do you need to use "+sxunc:" so that "flock LOCKFILE cmd with -options"
works as expected? How upstream tool handles it?
> + argc -= optind;
delete this line
> +
> + if (argv[0] == NULL)
> + bb_show_usage();
opt_complementary = "-1" will do this check.
> + if (command != NULL || argc > 1) {
s/argc > 1/argv[1]/
> + fd = open(argv[0], O_RDONLY|O_NOCTTY|O_CREAT, 0666);
> + if (fd < 0 && errno == EISDIR)
> + fd = open(argv[0], O_RDONLY|O_NOCTTY);
> + } else {
> + fd = atoi(argv[0]);
xatou_i?
> + }
> + if (fd < 0)
> + bb_perror_msg_and_die("cannot open: '%s'", argv[0]);
Use "can't open '%s'" string, the same string in many applets
is reused by linker.
> + argv++;
> + if (command == NULL)
> + command = argv[0];
> + if (command != NULL) {
Isn't it pointless to take a lock and exit (and thus lose lock)?
IOW: shouldn't you just disallow having no command?
> + pid_t pid;
> +
> + pid = fork();
NOMMU has no fork. See how other applets do this.
> + if (pid < 0)
> + bb_perror_msg_and_die("fork failed");
> + if (pid == 0) {
> + execvp(command, argv);
Error message?
> + exit(1);
> + }
> + waitpid(pid, &status, 0);
> + if (WIFEXITED(status))
> + status = WEXITSTATUS(status);
try using separate variable for exitcode. status var is on-stack,
and thus results in bigger code.
> + else if (WIFSIGNALED(status))
> + status = WTERMSIG(status) + 128;
> + else
> + status = 1;
> + }
> +
> + return status;
> +}
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
