9:53 a.m.
On 01/19/22 16:11, Richard W.M. Jones wrote:
On Wed, Jan 19, 2022 at 04:01:13PM +0100, Laszlo Ersek wrote:
> On 01/19/22 14:37, Richard W.M. Jones wrote:
>> module VCenterHTTPS = struct
>> + let to_string options args =
>> + let xs = args in
>> + let xs =
>> + match options.input_conn with
>> + | Some ic -> ("-ic " ^ ic) :: xs
>> + | None -> xs in
>> + let xs = "-i libvirt" :: xs in
>
> Huh, this was very non-intuitive. I thought "libvirt" here was an error,
> but it's not.
Unfortunately the virt-v2v command line is rather non-intuitive, but
we cannot really change it without breaking all users. It's actually
inherited from the old Perl virt-v2v (2009-2013) without much change.
My best advice is to follow the examples in the manual page as closely
as possible, and don't worry about it too much :-(
...
> Same here... I didn't realize the most frequently used transports for
> accessing vmware were libvirt based...
I think the original idea was that we could use libvirt to actually
access the disks as well as the metadata. That of course is not the
case, so we've now got this weird case where specific libvirt URLs are
carefully matched and turned into the real input modes:
https://github.com/libguestfs/virt-v2v/blob/f3180e3c0ef59ab52983903ff8060...
Yes, that's the code that convinced me that "-i libvirt" in the string
was OK!
When modularising virt-v2v I originally wanted to replace this with an
-im flag (input mode), but either it ended up breaking compatibility
for everyone, or we'd still need the old and new parsing together with
no real horizon for getting rid of the old parsing which is
complicated for no particular benefit.
So we are where we are.
>> diff --git a/output/output_json.ml b/output/output_json.ml
>> index bb0cdfeb5a..88fb4778d4 100644
>> --- a/output/output_json.ml
>> +++ b/output/output_json.ml
>> @@ -134,6 +134,12 @@ and json_path os output_name json_disks_pattern i =
>> module Json = struct
>> type t = unit
>>
>> + let to_string options =
>> + "-o json" ^
>> + match options.output_storage with
>> + | Some os -> " -os " ^ os
>> + | None -> ""
>> +
>> let setup dir options source =
>> let data = json_parse_options options in
>> let output_name = get_output_name options source in
>
> I tried to understand here whether "-oo json-disks-pattern" should be
> logged here; however, I can't tell the status of that option. The top of
> this file indicates the option is still supported, but the bottom of the
> file states the opposite ("no -oo (output options) are allowed here").
I think that's fixed if you pull
(commit cdde7864f65c7c3cf3400b978a52ade727402e17).
Ah indeed.
The purpose of the to_string method wasn't to serialize every option,
but to provide diagnostics in this specific case, so I didn't add
every input and output option in the commit under review.
OK!
>> + let to_string options = "-o openstack"
>> +
>> let setup dir options source =
>> let data = openstack_parse_options options in
>> let output_name = get_output_name options source in
>
> No need to print "-oo server-id=SERVER" and the other optional -oo
options?
(See above but) in this case it might be worth it because I guess we
are actually connecting to that server, and hence it would be useful
to know if it was incorrect.
[...]
> The code looks OK to me, but I'm not familiar enough with the various
> input and output modules to really claim that the set of options printed
> is -- or is not -- just right.
>
> Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
I think I'll add server-id to -o openstack.
Thanks!
Laszlo