On Tue, Aug 19, 2014 at 03:30:22AM +0100, Colin Watson wrote:
> Control: tag -1 fixed-upstream
>
> On Fri, Aug 15, 2014 at 01:47:36PM +0300, Riku Voipio wrote:
> > Qemu will start requiring setting P in binfmt_misc
> > flags [P]. Unfortunately update-binfmts doesn't just
> > pass flags section as-is.
>
> Right; this design decision was basically a gesture towards avoiding
> embedding details of the Linux kernel in update-binfmts' own interface
> in a way that would impede porting to other kernels later. But of
> course it's fine to extend update-binfmts like this where appropriate.
>
> Your patch looks fine to me, thanks, although I'll play with it a little
> before releasing a new version. I've pushed it to master. A couple of
> things that I tweaked when applying it, which don't require any action
> from you but just to let you know:
Right - the codebase was new to me so I didn't expect my patch to
be acceptable as is. Your changes make sense, thanks for taking time
to polish it!
Riku
> > diff --git a/src/update-binfmts.c b/src/update-binfmts.c
> > index 96ff1e0..8c11a86 100644
> > --- a/src/update-binfmts.c
> > +++ b/src/update-binfmts.c
> > @@ -362,6 +362,7 @@ static int act_enable (const char *name)
> > int need_detector;
> > const char *interpreter;
> > const char *flags;
> > + const char *preserve;
> > char *regstring;
> >
> > procdir_name = xasprintf ("%s/%s", procdir, name);
> > @@ -411,11 +412,13 @@ static int act_enable (const char *name)
> > /* Fake the interpreter if we need a userspace detector program. */
> > interpreter = need_detector ? run_detectors : binfmt->interpreter;
> >
> > + preserve = (binfmt->preserve && !strcmp (binfmt->preserve, "yes"))
> > + ? "P" : "";
> > flags = (binfmt->credentials && !strcmp (binfmt->credentials, "yes"))
> > ? "C" : "";
> > - regstring = xasprintf (":%s:%c:%s:%s:%s:%s:%s\n",
> > + regstring = xasprintf (":%s:%c:%s:%s:%s:%s:%s%s\n",
> > name, type, binfmt->offset, binfmt->magic,
> > - binfmt->mask, interpreter, flags);
> > + binfmt->mask, interpreter, flags, preserve);
> > if (test)
> > printf ("enable %s with the following format string:\n %s",
> > name, regstring);
>
> Better rename "flags" to "credentials" at this point. (Or construct a
> flags string based on both of these, but that means more run-time string
> manipulation so I think you're right to prefer this approach.)
>
> > @@ -827,7 +830,8 @@ enum opts {
> > OPT_ADMINDIR,
> > OPT_IMPORTDIR,
> > OPT_PROCDIR,
> > - OPT_TEST
> > + OPT_TEST,
> > + OPT_PRESERVE
> > };
> >
> > static struct argp_option options[] = {
>
> The ordering of this enum doesn't form part of a library ABI or
> anything, so OPT_PRESERVE should go in its natural place after
> OPT_CREDENTIALS rather than being put at the end.
>
> > @@ -863,6 +867,8 @@ static struct argp_option options[] = {
> > "use this userspace detector program" },
> > { "credentials", OPT_CREDENTIALS, "YES/NO", OPTION_HIDDEN,
> > "use credentials of original binary for interpreter (yes/no)" },
> > + { "preserve", OPT_PRESERVE, "YES/NO", OPTION_HIDDEN,
> > + "preserve argv0 of original binary for interpreter (yes/no)" },
> > { "package", OPT_PACKAGE, "PACKAGE-NAME", 0,
> > "for --install and --remove, specify the current package name", 1 },
> > { "admindir", OPT_ADMINDIR, "DIRECTORY", 0,
>
> Should be "argv[0]", matching the usual C spelling and the Linux
> kernel's documentation.
>
> Finally, this patch lacks any documentation, so I adjusted the manual
> page to document the new option.
>
> --
> Colin Watson [[email protected]]
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
