Sorry for the delay, I had 3 weeks vacation.
On Sat, Dec 08, 2012 at 12:19:12AM -0800, Andy Lutomirski wrote:
> +if BUILD_SETPRIV
> +usrbin_exec_PROGRAMS += setpriv
> +dist_man_MANS += sys-utils/setpriv.1
> +setpriv_SOURCES = sys-utils/setpriv.c
> +setpriv_LDADD = -lcap-ng
> +endif
Maybe the caps support should be optional, anyway you need to check
for the library in the configure script.
> +static void complain(const char *fmt, ...)
> +{
> + va_list ap;
> + va_start(ap, fmt);
> + vfprintf(stderr, fmt, ap);
> + va_end(ap);
> + fputc('\n', stderr);
> + usage(EXIT_FAILURE);
> +}
seems like poor reimplementation of err() from err.h :-)
> +// Returns the number of capabilities printed
please use /* comment */
> +static void stdout_perror(const char *prefix)
> +{
> + if (errno < 0 || errno >= sys_nerr)
> + printf(_("%s: error %d\n"), prefix, errno);
> + else
> + printf(_("%s: %s\n"), prefix, sys_errlist[errno]);
> +}
warnx() from err.h
> +static void dump_label(const char *name)
> +{
> + int fd = open("/proc/self/attr/current", O_RDONLY);
> + if (fd == -1) {
> + stdout_perror(name);
> + return;
> + }
> +
> + char buf[4097];
> + ssize_t len = read(fd, buf, sizeof(buf));
> + int e = errno;
we usually have declarations at the begin of block (function)...
> +struct options {
...and structs at the begin of the file. The name "options" seems
also too generic (like anything for getopt_long), what about
struct privcxt {
}
or so.
> bool nnp;
don't use bool in util-linux, within structs you can use bit arrays.
> // real and effective (in that order)
> bool have_uid[2];
> uid_t uid[2];
> bool have_gid[2];
> gid_t gid[2];
it would be more readable to use
uid_t ruid;
uid_t euid;
unsigned have_ruid:1,
have_euid:1;
(and the same for groups) rather than depend on an order.
> +static void priverr(const char *str)
> +{
> + perror(str);
> + exit(127);
> +}
err(127, str);
it would be also nice to have a macro (SETPRV_EX_* ?) in the code
rather than the magic number.
> +
> +static void parse_groups(struct options *opts, const char *str)
> +{
> + char *groups = strdup(str);
> + char *buf = groups; /* We'll reuse it */
> + char *c;
> +
> + opts->have_groups = true;
> + opts->num_groups = 0;
> + while ((c = strsep(&groups, ",")) != 0)
> + opts->num_groups++;
> +
> + // Start again
> + strcpy(buf, str); // It's exactly the right length
> + groups = buf;
> +
> + opts->groups = calloc(opts->num_groups, sizeof(gid_t));
> + size_t i = 0;
> + while ((c = strsep(&groups, ",")) != 0) {
> + char *end;
> + errno = 0;
> + long val = strtol(c, &end, 10);
strtol_or_err() (include/strutils.h)
> + if (!*c || *end || errno || val != (long long)(gid_t)val)
> + complain(_("Invalid supplementary group id"));
> + opts->groups[i++] = (gid_t)val;
> + }
> +
> + free(groups);;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + enum {
> + NNP = 256,
SETPRIV_OPT_NNP = CHAR_MAX + 1
> + RUID = 256,
> + EUID,
> + RGID,
> + EGID,
> + REUID,
> + REGID,
> + CLEAR_GROUPS,
> + KEEP_GROUPS,
> + GROUPS,
> + INHCAPS,
> + LISTCAPS,
> + CAPBSET,
> + SECUREBITS,
> + SELINUX_LABEL,
> + APPARMOR_PROFILE
> + };
[...]
> + if (RUID <= c && c <= REGID) {
> + /* This is easier than six independent cases. */
> + char *end;
> + errno = 0;
> + long val = strtol(optarg, &end, 10);
> + if (!*optarg || errno || *end)
> + complain(_("Failed to parse uid or gid"));
> +
> + if (c == REUID) {
if (opts.have_euid)
err(EXIT_FAILURE, _("duplicate euid");
opts.have_euid = 1;
opts.euid = strtol_or_err(optarg, _("failed to parse euid");
... no loop, no arrays for uids, just readable code.
[..]
> + if (dumplevel) {
> + if (total_opts != dumplevel || optind < argc)
> + complain(_("--dump is incompatible with all other options"));
> + dump(dumplevel);
> + return 0;
this is main(), so return EXIT_SUCCES;
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
