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 [cjwat...@debian.org] -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org