Microcom: new applet, mount: make better use of fstab

Denys Vlasenko vda.linux at googlemail.com
Wed Sep 12 12:06:56 UTC 2007


On Wednesday 12 September 2007 08:18, Vladimir Dronnikov wrote:
> Hello, Denis!
> 
> 1) microcom

+       // compose lock file name
+       if ( !device ) return ENODEV;
+       if ( strchr(device, '/') ) device = strrchr(device, '/')+1;
+       if ( !*device ) return ENODEV;
+       device_lock_file = xasprintf("/var/lock/LCK..%s", device);
+
+       // someone has already locked the device?
+       // ...yes? nothing doing
+       if ( !stat(device_lock_file, &st) ) {
+               return EEXIST;
+       }
+
+       // seems device is free -> lock it now!
+       fd = open(device_lock_file, O_CREAT | O_RDWR, 0640);
+       if ( fd < 0 ) {
+               return EPERM;
+       }
+       s = xasprintf("%10d\n", getpid());

This is racy, and lacks O_TRUNC too.

Don't stat. Simply use open(..., O_WRONLY|O_CREAT|O_TRUNC|O_EXCL, 0644)

If it fails, then file probably exists (because of O_EXCL).
If it succeeds, there was no file and you already created a new one.

Why "%10d" and not "%d"?

I also wonder what will happen if user *can* open /dev/ttyS0,
but can't create /var/lock/LCK..ttyS0 because of permissions.
Something like -f(orce) flag can be useful.

+       unlink(device_lock_file);
+       if (ENABLE_FEATURE_CLEAN_UP && device_lock_file) free(device_lock_file);

Superfluous check for device_lock_file != NULL. if device_lock_file == NULL,
unlink will segfault before you reach if().

+       if ( (errno=lock_device(argv[0])) ) {
+               bb_perror_msg("Can't lock device");


Why do you invent error code (EPERM)? The reason can be different
(say, /var/lock doesn't exist).
I think better way would be to explicitly say
"cannot create lock file FILE: <errno>" - less confusing.

+                       len = read(STDIN_FILENO, bb_common_bufsiz1, 100); if (len > 0) write(fd, bb_common_bufsiz1, len);

Not very readable.

+       do {
+               fd_set rfs;
+
+               FD_ZERO(&rfs);
+               FD_SET(STDIN_FILENO, &rfs);
+               FD_SET(fd, &rfs);
+
+               select(FD_SETSIZE, &rfs, NULL, NULL, NULL);

When you have fixed set of fds to check, code using poll
tends to ba smaller by ~100 bytes than select.
These FD_xxx macros produce nasty big code, and fd_set uses 128 bytes on stack.

+       puts("goodbye!\r\n");

Can this fool user into thinking that it came from remote machine?
--
vda



More information about the busybox mailing list