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:
> 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]
