Libguestfs - Jan 2014 - Re: [Bug 1046905] New: RFE: add argument to virt-sysprep to disable individual default operations

On Tuesday 07 January 2014 12:12:43 Richard W.M. Jones
wrote:
> On Tue, Jan 07, 2014 at 11:16:08AM +0100, Pino Toscano wrote:
> > On Friday 27 December 2013 10:58:15 you wrote:
> > > virt-sysprep either runs with all default operations or a
selected
> > > list of operations with the --enable argument.  A few times
I've
> > > found I'd like to use the default list, but minus one or two
> > > operations in particular, however there's no easy way to
specify
> > > this.
> > > 
> > > A --disable argument that took the default operation list and
> > > skipped
> > > selected operations would be useful.
> > 
> > A rough idea I had about this is adding a new --operations
> > parameter,
> > which would take a a comma-separated list of operations (just like
> > the current --enable), but with the following differences:
> > - a leading minus would disable the specified operation
> > - it would recognize the meta-keywords "all" (for all the
available
> > 
> >   operations) and "defaults" (for the ones enabled by
default)
> > 
> > Processing the list of operations would add/remove them from the
> > current set of operations, bailing out whether the resulting set is
> > empty (just like right now --enable rejects an empty string).
> > 
> > This way, you could write e.g.:
> > - --operations defaults,-hostname,user-account
> > 
> >   runs the default ones but not "hostname", and the
non-default
> >   "user-account"
> > 
> > - --operations all
> > 
> >   easy shortcut to run all the available operations
> > 
> > and so on.
> > 
> > --enable could just be an alias for --operations, or this new syntax
> > could be added directly to --enable directly.
> > 
> > Maybe I'm over-engineering, but IMHO seems a better way than
> > potentially adding a --disable argument and deal with the order of
> > --enable & --disable and their interactions.
> 
> Seems reasonable.
> 
> I guess only one --operations opt is allowed?
> 
> Or would we allow multiple and concatenate them together?  So that
> 
>   --operations foo,bar --operations -baz
> 
> would be equivalent to
> 
>   --operations foo,bar,-baz
> 
> ?
> 
> Multiple --operations could be useful for scripting, ie. it would let
> shell users write:
> 
>   opts="--operations all -a disk.img"
>   if [ $disable_foo ]; then opts="$opts --operations -foo"; fi
I agree, allowing multiple --operations is a good thing. After all, 
given the proposed behaviour, every --operation would alter the existing 
set of operations (empty at the first --operation invocation, as with
--enable), instead of starting from scratch every time.
> Another thing to test is whether the OCaml argument parser[1] can
> handle '--operations -foo' without thinking that -foo is a separate
> arg.
Ah yes, I read about the unsupported --foo=arg syntax.
OTOH, it seems that "--foo --foo", with --foo taking an Arg.String, 
gives "--foo" as value for it, so should be safe for us.

-- 
Pino Toscano

On Tuesday 07 January 2014 15:24:51 Pino Toscano wrote:
> On Tuesday 07 January 2014 12:12:43 Richard W.M. Jones wrote:
> > On Tue, Jan 07, 2014 at 11:16:08AM +0100, Pino Toscano wrote:
> > > On Friday 27 December 2013 10:58:15 you wrote:
> > > > virt-sysprep either runs with all default operations or a
> > > > selected
> > > > list of operations with the --enable argument.  A few times
I've
> > > > found I'd like to use the default list, but minus one or
two
> > > > operations in particular, however there's no easy way to
specify
> > > > this.
> > > > 
> > > > A --disable argument that took the default operation list
and
> > > > skipped
> > > > selected operations would be useful.
> > > 
> > > A rough idea I had about this is adding a new --operations
> > > parameter,
> > > which would take a a comma-separated list of operations (just
like
> > > the current --enable), but with the following differences:
> > > - a leading minus would disable the specified operation
> > > - it would recognize the meta-keywords "all" (for all
the
> > > available
> > > 
> > >   operations) and "defaults" (for the ones enabled by
default)
> > > 
> > > Processing the list of operations would add/remove them from the
> > > current set of operations, bailing out whether the resulting set
> > > is
> > > empty (just like right now --enable rejects an empty string).
> > > 
> > > This way, you could write e.g.:
> > > - --operations defaults,-hostname,user-account
> > > 
> > >   runs the default ones but not "hostname", and the
non-default
> > >   "user-account"
> > > 
> > > - --operations all
> > > 
> > >   easy shortcut to run all the available operations
> > > 
> > > and so on.
> > > 
> > > --enable could just be an alias for --operations, or this new
> > > syntax
> > > could be added directly to --enable directly.
> > > 
> > > Maybe I'm over-engineering, but IMHO seems a better way than
> > > potentially adding a --disable argument and deal with the order
of
> > > --enable & --disable and their interactions.
> > 
> > Seems reasonable.
Attached there is a first PoC for --operations, implementing the idea I 
outlined with your hints too.

Possibly not the best ocaml code ever (my first long-ish ocaml code, 
actually), but seems to work fine; maybe the various functions added in 
Sysprep_operations could be simplified in one with a label as parameter 
(eg `All | `Defaults).

-- 
Pino Toscano

On Thu, Jan 09, 2014 at 04:21:10PM +0100, Pino Toscano
wrote:
> +  and set_operations op_string > +    let currentopset > +     
match (!operations) with
No need for parentheses around !operations.
> +        let n = ref op_name in
> +        let remove = string_prefix op_name "-" in
> +        if remove then
> +          n := String.sub op_name 1 (String.length op_name - 1);
> +        match !n with
This can be written a bit more naturally as ...

  let n, remove     if string_prefix op_name "-" then
      String.sub op_name 1 (String.length op_name-1), true
    else
      op_name, false in
  match n with
  ...
> +          let f = if remove then
Sysprep_operation.remove_defaults_from_set else
Sysprep_operation.add_defaults_to_set in
> +          f opset
You can actually write:

  (if remove then Sysprep_operation.remove_defaults_from_set
   else Sysprep_operation.add_defaults_to_set) opset

I don't know which way you think is clearer, but I would avoid the >80
character lines.
> +    "--operations", Arg.String set_operations, " " ^
s_"Enable/disable specific operations";
I'd also add an alias "--operation" (it would just do the same
thing).
> +let add_list_to_opset l s > +  List.fold_left (
> +    fun acc elem ->
> +      OperationSet.add elem acc
> +  ) s l
> +
> +let remove_list_from_opset l set > +  OperationSet.fold (
> +    fun elem opset ->
> +      if List.exists (
> +        fun li ->
> +          li.name = elem.name
> +      ) l then
> +        opset
> +      else
> +        OperationSet.add elem opset
> +  ) set empty_set
OCaml Set has subset, intersection, difference operations if you need
them.  See /usr/lib64/ocaml/set.mli

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW