Re: [PATCH] virsh: add --domain option for domain-to-native

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

 



On Mon, May 29, 2017 at 05:50:46PM -0300, Julio Faracco wrote:
> It would be nice to include the changes into documentation, wouldn't it?
> tools/virsh.pod has an entry for example.
> 
Many thanks for pointing it out. I modified man page by editing virsh.pod
in v3 patch.

Dan
> 2017-05-29 16:40 GMT-03:00 Dan <srwx4096@xxxxxxxxx>:
> > On Mon, May 29, 2017 at 08:53:18PM +0200, Martin Kletzander wrote:
> >> On Mon, May 29, 2017 at 02:25:03PM -0400, Dan wrote:
> >> > On Tue, May 16, 2017 at 11:41:55AM +0200, Martin Kletzander wrote:
> >> > > On Mon, May 15, 2017 at 04:29:48AM -0400, Daniel Liu wrote:
> >> > > > Fix bug 835476[1].
> >> > >
> >> > > It's enough to mention it below (as you did).
> >> > >
> >> > > > virsh: add [--domain DOMAIN] option to  domxml-to-native DOMAIN COMMAND
> >> > >
> >> > > This first line is already in the subject.
> >> > >
> >> > > > Add support for the following syntax:
> >> > > > domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports
> >> > > > either designating domain (domain id, uuid, or name), or path to XML domain
> >> > > > configuration file.
> >> > > >
> >> > >
> >> > > I would reword this a little bit.  How would you feel about something
> >> > > along the lines of:
> >> > >
> >> > >  The option allows someone to run domain-to-native on already existing
> >> > >  domain without the need of supplying their XML.  It is basically
> >> > >  wrapper around `virsh dumpxml $dom | virsh domxml-to-native /dev/stdin`.
> >> > >
> >> > >  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476
> >> > >
> >> > I made changes accordingly in the version 2 of this patch.
> >> > > > E.g.:
> >> > > > virsh domxml-to-native qemu-argv --domain RHEL7.3   # domain name
> >> > > > virsh domxml-to-native qemu-argv --domain 10        # domain id
> >> > > > virsh domxml-to-native qemu-argv dumped_dom.xml     # dumped xml
> >> > > >
> >> > > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=835476
> >> > > > ---
> >> > > > tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------
> >> > > > 1 file changed, 44 insertions(+), 10 deletions(-)
> >> > > >
> >> > > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> >> > > > index 0d19d0e01..a79fd3ab2 100644
> >> > > > --- a/tools/virsh-domain.c
> >> > > > +++ b/tools/virsh-domain.c
> >> > > > @@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> >> > > >      .flags = VSH_OFLAG_REQ,
> >> > > >      .help = N_("target config data type format")
> >> > > >     },
> >> > > > +    {.name = "domain",
> >> > > > +     .type = VSH_OT_DATA,
> >> > > > +     .flags = VSH_OFLAG_REQ_OPT,
> >> > > > +     .help = N_("domain name, id or uuid")
> >> > > > +    },
> >> > > >     {.name = "xml",
> >> > > >      .type = VSH_OT_DATA,
> >> > > > -     .flags = VSH_OFLAG_REQ,
> >> > > >      .help = N_("xml data file to export from")
> >> > > >     },
> >> > > >     {.name = NULL}
> >> > > > @@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> >> > > > static bool
> >> > > > cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
> >> > > > {
> >> > > > -    bool ret = true;
> >> > > > +    bool ret = false;
> >> > > >     const char *format = NULL;
> >> > > > -    const char *xmlFile = NULL;
> >> > > > -    char *configData;
> >> > > > -    char *xmlData;
> >> > > > +    const char *domain = NULL;
> >> > > > +    const char *xml = NULL;
> >> > > > +    char *xmlData = NULL;
> >> > > > +    char *configData = NULL;
> >> > > >     unsigned int flags = 0;
> >> > > >     virshControlPtr priv = ctl->privData;
> >> > > > +    virDomainPtr dom = NULL;
> >> > > >
> >> > > > -    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> >> > > > -        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
> >> > >
> >> > > If this was already there, you could keep using it.  But that's not a
> >> > > big deal for me, just some others might not like it.
> >> > >
> >> > I kept the existing code and only added an additional check for "domain"
> >> > in v2, i.e.:
> >> >
> >> >    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> >> >        vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 ||
> >> >        vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
> >> >        return false;
> >> > > > +    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
> >> > > > +        return false;
> >> > > > +
> >> > > > +    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
> >> > > > +        return false;
> >> > > > +
> >> > >
> >> > > [1] So here you get the domain name/id/uuid ...
> >> > >
> >> > > > +    if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)
> >> > > >         return false;
> >> > > >
> >> > > > -    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
> >> > > > +    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
> >> > > > +
> >> > > > +    if (domain)
> >> > > > +        dom = virshCommandOptDomain(ctl, cmd, &domain);
> >> > > > +
> >> > >
> >> > > ... and here you get the object.  What do you supply as the third
> >> > > parameter?  Check what the function does.  There's a leak that you will
> >> > > fix by getting rid of the lines above [1].  And just supply NULL here.
> >> > >
> >> > I did not fully get it by "getting rid of the lines above [1]." But I
> >>
> >> I meant the three lines before the [1] (I used that number in square
> >> brackets as a link.  That's why I said "Check what the function does".
> >
> > So I deleted the line checking "domain" leaving origin conditional like
> > as it was:
> >
> >  if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> >         vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
> >
> > However, this change would reulst in error when supplying "domain" as the
> > string of domain needs to be accquired, which was achieved by calling
> > vshCommandOptStringReq(ctl, cmd, "domain", &domain).
> >
> > So far, the exception handeling and normal return are the following:
> >
> >
> > $ virsh -c qemu:///system list --all
> >  Id    Name                           State
> > ----------------------------------------------------
> >  2     ubuntu1404                     running
> >  -     RHEL7.3                        shut off
> >
> > $ virsh -c qemu:///system domxml-to-native
> > error: command 'domxml-to-native' requires <format> option
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv
> > error: need either domain (ID, UUID, or name) or domain XML configuration file path
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv 2
> > error: Failed to open file '2': No such file or directory
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --domain 2 => OK
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170422.xml  => OK
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170529.xml # Wrong XML
> > error: convert from domain XML to native command failed
> > error: (domain_definition):111: expected '>'
> > </domain>
> > -------^
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml => OK
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain
> > error: expected syntax: --domain <string>
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain 3
> > error: Options --domain and --xml are mutually exclusive
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --domain 3
> > error: failed to get domain '3'
> > error: Domain not found: no domain with matching name '3'
> >
> > Best,
> >
> > Dan
> >
> > --
> > 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