[Libguestfs] [PATCH v2] rescue: add --autosysroot option RHBZ#1183493

[Pino Toscano]

On Thursday 16 June 2016 16:13:24 Maros Zatko wrote:
> On 06/09/2016 03:18 PM, Pino Toscano wrote:
> > In data mercoledì 1 giugno 2016 02:04:33, Maros Zatko ha scritto:
> >> --autosysroot option uses suggestions to user on how to mount filesystems
> >> and change root suggested by --suggest option in virt-rescue.
> > IMHO it should be called -i, like in the other tools, as what
> > --autosysroot does is basically the same.
> We can call it -i, I don't mind, I've implemented this as was in RFE.

That's a suggestion of course :) It's our task to make sure new features
are properly integrate with the rest of the implementation.

> >> Commands are passed on kernel command line in format
> >> guestfs_command=command;. Command ends with a semicolon and there can be
> >> multiple commands specified. These are executed just before bash starts.
> > Passing arbitrary commands might not be the best idea ever -- other than
> > potentially running into issues such as quoting and escaping, the length
> > of the boot command line of the Linux kernel is limited [1], and we are
> > already using more than 200 characters.
> What about escaping, have you found a counter-example?

You are not quoting nor escaping the commands added, for example.

> I don't see much problem with this limit either, have we run into an 
> issue? It seems that
> max allowed size is about 4KiB (between 256 and 4KiB) and we're running a VM
> so I don't ever expect this to be a problem.

Exceeding the limits means our arguments will be cut off, so things
will not work as expected because less arguments are read/used.

> > An alternative approach with the cmdline could be passing like
> > guestfs_mount=/dev:/mntpoint:opts, which is slightly more complex to
> > parse, although provide a more strict interface and requires no eval.
> >
> > [1] https://www.kernel.org/doc/Documentation/kernel-parameters.txt
> Hm, this introduces another type of splitter (:) and we would have to 
> think about escaping it too
> (but this time it's not done by bash itself) - what if : is included in 
> filename?

We already use this syntax in -m/--mount parameters of various tools.

> >> On successfull run user is presented directly with bash in chroot
> >> environment.
> > I'd not do that -- one of the way virt-rescue is useful is work on
> > the guest using the tools available in the appliance, so having an
> > an option to mount all the filesystems while still be in the appliance
> > would be the optimal thing to do IMHO.
> I don't get this, --autosysroot is meant to be "optimization" so one 
> doesn't have to copy-paste
> any commands and this feature is what was requested in RFE, and user 
> doesn't have to use this automagic.

Sure, however mounting and chroot means you have no benefit if you want
to only mount everything but still work outside the guest.  Since
chroot is just a single command which can be suggested before running
bash, IMHO it'd be much better to automate what requires more manual
work (inspection + mount).

> >> -int
> >> -main (int argc, char *argv[])
> >> -{
> >> -  setlocale (LC_ALL, "");
> >> -  bindtextdomain (PACKAGE, LOCALEBASEDIR);
> >> -  textdomain (PACKAGE);
> >> -
> >> -  parse_config ();
> >> +struct drv *drvs = NULL;
> >> +struct drv *drv;
> >> +char **cmds = NULL;
> >> +const char *format = NULL;
> >> +bool format_consumed = true;
> >> +int c;
> >> +int option_index;
> >> +int network = 0;
> >> +char *append = NULL;
> >> +int memsize = 0;
> >> +int smp = 0;
> >> +int suggest = 0;
> >> +int autosysroot = 0;
> > Any reason these are turned into global variables?
> These are filled in option parser and used elsewhere, it didn't
> seem to make sense to make it more complicated for this simple program.
> These were used as if they were global variables before, difference was 
> that they were used
> in one large function (main) and their initialization was not factored 
> out to function so
> their 'global' sense/semantics didn't really change.

My point was about whether the actual change in this patch was really
needed -- having patches doing specific changes instead of mixing
different changes together helps in reviewing and debugging.

> >> +      memsize = 0;
> >> +      smp = 0;
> >> +      suggest = 0;
> >> +      autosysroot = 0;
> >> +
> >> +      /* Create new guestfs handle, this time for rescue instead of OS detection */
> >> +      g = guestfs_create ();
> >> +      /* Parse options with --suggest flag removed */
> >> +      parse_opts (newcount, args);
> >> +      argc = newcount;
> > I'm puzzled -- why do you need to parse the arguments again? You have
> > all the options already, so just create a new handle, and set the
> > options for it.
> Because we need to reinitialize stuff - library and so - and options 
> should be constructed a bit different when
> we want to just inspect and when we need r/w access.

The current option parsing already reads everything in variables
(except the selinux option, but that be easily changed), so all you
need to do is creating a new handle and setting up everything.
Other than the read_only behaviour (which does not depend on the
options, but on a global variable), what are differences do you see?

Thanks,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160617/c57bd6d7/attachment.sig>