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

Re: [libvirt] [PATCH] virsh: new net-update command



On Wed, Sep 19, 2012 at 12:32:18PM -0600, Eric Blake wrote:
> On 09/19/2012 12:08 PM, Laine Stump wrote:
> > This new virsh command uses the new virNetworkUpdate() API to modify
> > an existing network definition, and optionally have those
> > modifications take effect immediately without restarting the network.
> > 
> 
> > +
> > +static const vshCmdOptDef opts_network_update[] = {
> > +    {"network", VSH_OT_DATA, VSH_OFLAG_REQ, N_("network name or uuid")},
> > +    {"command", VSH_OT_DATA, VSH_OFLAG_REQ,
> > +     N_("type of update (add, delete, or modify)")},
> 
> s/add/add-first, add-last/
> 
> > +    {"section", VSH_OT_DATA, VSH_OFLAG_REQ,
> > +     N_("which section of network configuration to update")},
> > +    {"xml", VSH_OT_BOOL, 0, N_("xml is specified directly on commandline")},
> > +    {"file", VSH_OT_BOOL, 0, N_("file containing xml is specified on commandline")},
> > +    {"xmldata", VSH_OT_DATA, VSH_OFLAG_REQ,
> > +     N_("complete xml element (or name of file containing xml) to add/modify, "
> > +        "or to be matched for search")},
> 
> Interesting choice to make --xml and --file be boolean flags, and
> '[--xmldata] data' be the string that becomes either the file name or
> the xml content.  I might have done just two optional VSH_OT_DATA
> arguments for --xml and --file and then manually checked that exactly
> one of the two was supplied, instead of using three arguments.  But what
> you did works, so no need to change it.

Yes, this is total overkill. When I suggested --xmldata as an option
my intent was that you'd have the following usage

   virsh net-update  /path/to/xmlfile

   virsh net-update --xmldata "some XML data inline"

IMHO, requiring --xml and --file in addition is unfriendly to the
user too


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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