Re: [PATCH v4] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jun 26, 2017 at 09:30:11PM -0300, Julio Faracco wrote:
> Hi Dan,
> 
> Have you tested the --xml argument as VSH_OT_ARGV?
>      {.name = "domain",
> -     .type = VSH_OT_DATA,
> +     .type = VSH_OT_STRING,
>       .flags = VSH_OFLAG_REQ_OPT,
>       .help = N_("domain name, id or uuid")
>      },
>      {.name = "xml",
> -     .type = VSH_OT_DATA,
> +     .type = VSH_OT_ARGV,
>       .help = N_("xml data file to export from")
> 
> Check the command 'domfsthaw', option 'mountpoint'.
> This is the best approach that I got until now.
> 
Yes, it is working! Finally I understand it.
When an --optionname is optional for a flag, VSH_OT_ARGV should be used.
Similarly another such case is schedinfo's --set flag. Yeah, I should be
more careful. Thank you very much.
Shall I send a new patch using your apprach?

Now domxml-to-native --help looks like:

 NAME                                        
    domxml-to-native - Convert domain XML to native config                                   

  SYNOPSIS                                    
    domxml-to-native <format> [--domain <string>] [[--xml] <string>]...                      

  DESCRIPTION                                 
    Convert domain XML config to a native guest configuration format.                        

  OPTIONS                                     
    [--format] <string>  target config data type format                                      
    --domain <string>  domain name, id or uuid                                               
    [--xml] <string>  xml data file to export from   

Just for aesthetic reason, shall I switch the order of the last two
lines? (though I don't see a strong point to do this).

Thanks,

Dan
> 2017-06-26 17:21 GMT-03:00 Dan <srwx4096@xxxxxxxxx>:
> > On Sun, Jun 25, 2017 at 08:33:39AM -0400, John Ferlan wrote:
> >>
> >>
> >> On 06/24/2017 08:05 PM, Julio Faracco wrote:
> >> > My apologies. I didn't read the function logic.
> >> > The second error was introduced by my "fix".
> >> > The error with help parameter is happening.
> >> >
> >> > If --domain is required, I believe that moving to STRING
> >> > is enough. As I said, VSH_OT_DATA requires
> >> > VSH_OFLAG_REQ. See vsh.c line 746.
> >> >
> >> >      {.name = "domain",
> >> > -     .type = VSH_OT_DATA,
> >> > +     .type = VSH_OT_STRING,
> >> >       .flags = VSH_OFLAG_REQ_OPT,
> >> >       .help = N_("domain name, id or uuid")
> >> >      },
> >> >      {.name = "xml",
> >> > -     .type = VSH_OT_DATA,
> >> > +     .type = VSH_OT_STRING,
> >> >       .help = N_("xml data file to export from")
> >> >
> >>
> >> ugh... Could you send a patch please. I do recall at one point wondering
> >> what the downside of removing VSH_OFLAG_REQ from "xml" .flags was, no I
> >> know. Of course I probably won't remember the next time either
> >>
> >> Tks, -
> >>
> >> John
> >>
> > I think we may need to modify vshCmddefHelp() in vsh.c to really make it
> > look like as expected. For example, in some case, the SYNOPSIS part also
> > displayed incorrectly.
> >
> > SYNOPSIS
> >  domxml-to-native <format> <xml> <domain>
> >
> >
> > Instead, the following is similar to what we want:
> >
> > SYNOPSIS-
> >  domxml-to-native <format> { [--xml] <xml> | --domain <domain> }
> >
> > Current implementation of vshCmddefHelp() does not handle this
> > "complicated" logic. The question is whether we want to modify it or
> > whether we want to spend the effort modifying it?
> >
> > Dan
> >> > 2017-06-24 17:54 GMT-03:00 Julio Faracco <jcfaracco@xxxxxxxxx>:
> >> >> Hi guys,
> >> >>
> >> >> I updated the source this weekend, I missed the ability of calling help.
> >> >> virsh # domxml-to-native --help
> >> >>   NAME
> >> >>     domxml-to-native - Convert domain XML to native config
> >> >>
> >> >>   SYNOPSIS
> >> >>     domxml-to-native <format> [<domain>] [<xml>]
> >> >>
> >> >>   DESCRIPTION
> >> >>     Convert domain XML config to a native guest configuration format.
> >> >>
> >> >>   OPTIONS
> >> >>     [--format] <string>  target config data type format
> >> >> error: internal error: bad options in command: 'domxml-to-native'
> >> >>
> >> >>
> >> >> This is why:
> >> >> --- a/tools/virsh-domain.c
> >> >> +++ b/tools/virsh-domain.c
> >> >> @@ -9858,11 +9858,11 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> >> >>      },
> >> >>      {.name = "domain",
> >> >>       .type = VSH_OT_DATA,
> >> >> -     .flags = VSH_OFLAG_REQ_OPT,
> >> >> +     .flags = VSH_OFLAG_REQ,
> >> >>       .help = N_("domain name, id or uuid")
> >> >>      },
> >> >>      {.name = "xml",
> >> >> -     .type = VSH_OT_DATA,
> >> >> +     .type = VSH_OT_STRING,
> >> >>       .help = N_("xml data file to export from")
> >> >>      },
> >> >>      {.name = NULL}
> >> >>
> >> >> VSH_OT_DATA requires VSH_OFLAG_REQ.
> >> >> So, since XML is not required...
> >> >> This diff fits this case. But I'm still confused.
> >> >> Because I cannot check my XML files right now.
> >> >>
> >> >> virsh # domxml-to-native qemu-argv /home/julio/WINDOWS_7.xml
> >> >> error: failed to get domain '/home/julio/WINDOWS_7.xml'
> >> >> error: Domain not found: no domain with matching name
> >> >> '/home/julio/WINDOWS_7.xml'
> >> >>
> >> >> 2017-06-23 6:38 GMT-03:00 Martin Kletzander <mkletzan@xxxxxxxxxx>:
> >> >>> On Thu, Jun 22, 2017 at 06:21:49PM -0400, John Ferlan wrote:
> >> >>>>
> >> >>>>
> >> >>>> [...]
> >> >>>>
> >> >>>>>>>>
> >> >>>>>>>
> >> >>>>>>> There was no change, it is an additional variable, the original one is
> >> >>>>>>> below.  The number of differences would be the same, I believe.
> >> >>>>>>>
> >> >>>>>>
> >> >>>>>> If edit the file and change "xml" to "xmlFile" and change the 3 changed
> >> >>>>>> xml variable references things work... Like I said, nit, IDC if it's
> >> >>>>>> changed or not...
> >> >>>>>>
> >> >>>>>
> >> >>>>> My bad, I misread that, you're right.
> >> >>>>
> >> >>>>
> >> >>>> In order to "close" on this, if a squash the attach patch does that work
> >> >>>> for everyone?
> >> >>>>
> >> >>>> John
> >> >>>
> >> >>>
> >> >>> WFM
> >> >>>
> >> >>> Reviewed-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> >> >>>
> >> >>> --
> >> >>> libvir-list mailing list
> >> >>> libvir-list@xxxxxxxxxx
> >> >>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux