[libvirt] [PATCH] virsh: new net-update command
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. An example usage: virsh net-update add-last ip-dhcp-host \ --xml "<host mac='00:11:22:33:44:55' ip='192.168.122.45'/>" \ --live --config If you like, you can instead put the xml into a file, and call like this: virsh net-update add-last ip-dhcp-host \ --file "<host mac='00:11:22:33:44:55' ip='192.168.122.45'/>" \ --live --config (or just leave out the "--file", since that is the default). --live, --config, and --current options - if you specify --live, only the live state of the network will be updated. If you also specify --config, then the persistent configuration will also be updated; these two commands can be given separately, or both together. If you don't specify either (you can optionally specify "--current" for the same effect), then the "current" config will be updated (i.e. if the network is active, then only its live config is affected, but if the network is inactive, only the persistent config is affected). A --parent-index option is also available (to give the index within a list of parent objects, e.g. the index of the parent <ip> element when updating ip-dhcp-host elements), but is optional and at least for now will probably be used rarely. --- tools/virsh-network.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 2c32a78..64be260 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -737,6 +737,163 @@ cmdNetworkUndefine(vshControl *ctl, const vshCmd *cmd) } /* + * "net-update" command + */ +static const vshCmdInfo info_network_update[] = { + {"help", N_("update parts of an existing network's configuration")}, + {"desc", ""}, + {NULL, NULL} +}; + +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)")}, + {"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")}, + {"parent-index", VSH_OT_INT, 0, N_("which parent object to search through")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, + {NULL, 0, 0, NULL} +}; + +VIR_ENUM_DECL(virNetworkUpdateCommand) +VIR_ENUM_IMPL(virNetworkUpdateCommand, VIR_NETWORK_UPDATE_COMMAND_LAST, + "none", "modify", "delete", "add-last", "add-first"); + +VIR_ENUM_DECL(virNetworkSection) +VIR_ENUM_IMPL(virNetworkSection, VIR_NETWORK_SECTION_LAST, + "none", "bridge", "domain", "ip", "ip-dhcp-host", + "ip-dhcp-range", "forward", "forward-interface", + "forward-pf", "portgroup", "dns-host", "dns-txt", + "dns-srv"); + +static bool +cmdNetworkUpdate(vshControl *ctl, const vshCmd *cmd) +{ + bool ret = false; + virNetworkPtr network; + const char *commandStr = NULL; + const char *sectionStr = NULL; + int command, section, parentIndex = -1; + const char *xmldata = NULL; + char *xmldataFromFile = NULL; + bool isFile = vshCommandOptBool(cmd, "file"); + bool isXml = vshCommandOptBool(cmd, "xml"); + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + unsigned int flags = 0; + const char *affected; + + if (!(network = vshCommandOptNetwork(ctl, cmd, NULL))) + goto cleanup; + + if (vshCommandOptString(cmd, "command", &commandStr) < 0) { + vshError(ctl, "%s", _("missing or malformed command argument")); + goto cleanup; + } + command = virNetworkUpdateCommandTypeFromString(commandStr); + if (command <= 0 || command >= VIR_NETWORK_UPDATE_COMMAND_LAST) { + vshError(ctl, _("unrecognized command name '%s'"), commandStr); + goto cleanup; + } + + if (vshCommandOptString(cmd, "section", §ionStr) < 0) { + vshError(ctl, "%s", _("missing or malformed section argument")); + goto cleanup; + } + section = virNetworkSectionTypeFromString(sectionStr); + if (section <= 0 || section >= VIR_NETWORK_SECTION_LAST) { + vshError(ctl, _("unrecognized section name '%s'"), sectionStr); + goto cleanup; + } + + if (vshCommandOptInt(cmd, "parent-index", &parentIndex) < 0) { + vshError(ctl, "%s", _("malformed parent-index argument")); + goto cleanup; + } + + /* the goal is to have a full xml element in the "xmldata" + * string. This can either be directly given with the --xml + * option, or indirectly with the --file option (default, + * indicates that the xmldata on the commandline is really the + * name of a file whose contents are the desired xml). + */ + + if (!isXml) + isFile = true; + if (isXml && isFile) { + vshError(ctl, "%s", _("you can specify file (default) or xml, " + "but not both")); + goto cleanup; + } + if (vshCommandOptString(cmd, "xmldata", (const char **)&xmldata) < 0) { + vshError(ctl, "%s", _("malformed xmldata argument")); + goto cleanup; + } + + if (isFile) { + /* contents of xmldata is actually the name of a file that + * contains the xml. + */ + if (virFileReadAll(xmldata, VSH_MAX_XML_FILE, &xmldataFromFile) < 0) + return false; + /* NB: the original xmldata is freed by the vshCommand + * infrastructure, so it's safe to lose its pointer here. + */ + xmldata = xmldataFromFile; + } + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags |= VIR_NETWORK_UPDATE_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG; + if (live) + flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE; + } + + if (virNetworkUpdate(network, command, + section, parentIndex, xmldata, flags) < 0) { + vshError(ctl, _("Failed to update network %s"), + virNetworkGetName(network)); + goto cleanup; + } + + if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { + if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE) + affected = _("persistent config and live state"); + else + affected = _("persistent config"); + } else if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE) { + affected = _("live state"); + } else if (virNetworkIsActive(network)) { + affected = _("live state"); + } else { + affected = _("persistent config"); + } + + vshPrint(ctl, _("Updated network %s %s"), + virNetworkGetName(network), affected); + ret = true; +cleanup: + virNetworkFree(network); + VIR_FREE(xmldataFromFile); + return ret; +} + +/* * "net-uuid" command */ static const vshCmdInfo info_network_uuid[] = { @@ -854,6 +1011,7 @@ const vshCmdDef networkCmds[] = { {"net-start", cmdNetworkStart, opts_network_start, info_network_start, 0}, {"net-undefine", cmdNetworkUndefine, opts_network_undefine, info_network_undefine, 0}, + {"net-update", cmdNetworkUpdate, opts_network_update, info_network_update, 0}, {"net-uuid", cmdNetworkUuid, opts_network_uuid, info_network_uuid, 0}, {NULL, NULL, NULL, NULL, 0} }; -- 1.7.11.4
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.
+ /* the goal is to have a full xml element in the "xmldata" + * string. This can either be directly given with the --xml + * option, or indirectly with the --file option (default, + * indicates that the xmldata on the commandline is really the + * name of a file whose contents are the desired xml). + */ + + if (!isXml) + isFile = true; + if (isXml && isFile) { + vshError(ctl, "%s", _("you can specify file (default) or xml, " + "but not both")); + goto cleanup; + } + if (vshCommandOptString(cmd, "xmldata", (const char **)&xmldata) < 0) {
This cast shouldn't be necessary.
+ vshError(ctl, "%s", _("malformed xmldata argument")); + goto cleanup; + } + + if (isFile) { + /* contents of xmldata is actually the name of a file that + * contains the xml. + */ + if (virFileReadAll(xmldata, VSH_MAX_XML_FILE, &xmldataFromFile) < 0) + return false; + /* NB: the original xmldata is freed by the vshCommand + * infrastructure, so it's safe to lose its pointer here.
Misleading comment; rather, xmldata is a 'const char *' and doesn't need to be freed. However, there's one major omission preventing me acking this: virsh.pod needs to document the new command. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/19/2012 02:32 PM, 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/
Right. I also just added manual recognition of "add" as a synonym for "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.
That's how I originally did it, but I didn't like being required to always say "--file xyz.xml" (while the existing commands that take a filename don't require that - the filename option can be positionally determined). Dan gave me the idea of having the (optionally unnamed) string option, along with two booleans. Of course, "--file" is kind of pointless, since that's the default behavior anyway. I'm kind of on the fence between the two methods right now - whether to be more compatible with existing command syntax, or have fewer options. I realize that one problem of having the two booleans is that if somebody wants to specify the args in a different order, they would need to use the name of the string option, and it would end up looking something like this: net-update add --file --xmldata myfile.xml --section ip-dhcp-host --parent-index 4 --live (Likely nobody would ever knowingly choose to do it that way, but...) So, the two possibilities are: 1) two string options, --file and --xml, so the command would look like one of these: net-update add ip-dhcp-host --file xyz.xml --live net-update add ip-dhcp-host --xml "<host mac='x:x:x:x:x:x' ip='1.2.3.4'/>" 2) two booleans and a string which is mandatory, but can be unnamed if in the right position: net-update add ip-dhcp-host xyz.xml --live net-update add ip-dhcp-host --xml "<host mac='x:x:x:x:x:x' ip='1.2.3.4'/>" Any votes for one or the other?
+ /* the goal is to have a full xml element in the "xmldata" + * string. This can either be directly given with the --xml + * option, or indirectly with the --file option (default, + * indicates that the xmldata on the commandline is really the + * name of a file whose contents are the desired xml). + */ + + if (!isXml) + isFile = true; + if (isXml && isFile) { + vshError(ctl, "%s", _("you can specify file (default) or xml, " + "but not both")); + goto cleanup; + } + if (vshCommandOptString(cmd, "xmldata", (const char **)&xmldata) < 0) { This cast shouldn't be necessary.
Yes, that was leftover from interim hacking when I was trying to use the same pointer for vshCommandOptString() and virFileReadAll().
+ vshError(ctl, "%s", _("malformed xmldata argument")); + goto cleanup; + } + + if (isFile) { + /* contents of xmldata is actually the name of a file that + * contains the xml. + */ + if (virFileReadAll(xmldata, VSH_MAX_XML_FILE, &xmldataFromFile) < 0) + return false; + /* NB: the original xmldata is freed by the vshCommand + * infrastructure, so it's safe to lose its pointer here. Misleading comment; rather, xmldata is a 'const char *' and doesn't need to be freed.
Well, it points to data that is in an arg that's parsed/allocated by vshCommandParse, and later freed by vshCommandFree, so both are correct statements. I'll change it to something that makes us both happy :-)
However, there's one major omission preventing me acking this: virsh.pod needs to document the new command.
Oops. Yeah, I'll do that before I submit it again.
On 09/19/2012 06:04 PM, Laine Stump wrote:
+ {"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.
That's how I originally did it, but I didn't like being required to always say "--file xyz.xml" (while the existing commands that take a filename don't require that - the filename option can be positionally determined). Dan gave me the idea of having the (optionally unnamed) string option, along with two booleans. Of course, "--file" is kind of pointless, since that's the default behavior anyway.
I'm not strongly tied to either approach, so does anyone else want to chime in?
I'm kind of on the fence between the two methods right now - whether to be more compatible with existing command syntax, or have fewer options. I realize that one problem of having the two booleans is that if somebody wants to specify the args in a different order, they would need to use the name of the string option, and it would end up looking something like this:
net-update add --file --xmldata myfile.xml --section ip-dhcp-host --parent-index 4 --live
This example's missing '--network net' as well, but yes, it illustrates the point.
(Likely nobody would ever knowingly choose to do it that way, but...)
So, the two possibilities are:
1) two string options, --file and --xml, so the command would look like one of these:
net-update add ip-dhcp-host --file xyz.xml --live net-update add ip-dhcp-host --xml "<host mac='x:x:x:x:x:x' ip='1.2.3.4'/>"
2) two booleans and a string which is mandatory, but can be unnamed if in the right position:
net-update add ip-dhcp-host xyz.xml --live net-update add ip-dhcp-host --xml "<host mac='x:x:x:x:x:x' ip='1.2.3.4'/>"
Option 3 - have a single mandatory string argument, then treat it as XML if it begins with '<' and as a filename otherwise (and if you happen to have a file in the current directory starting with '<', then prefix it with './').
Any votes for one or the other?
Sorry I'm not being more decisive on this one.
+ if (virFileReadAll(xmldata, VSH_MAX_XML_FILE, &xmldataFromFile) < 0) + return false; + /* NB: the original xmldata is freed by the vshCommand + * infrastructure, so it's safe to lose its pointer here. Misleading comment; rather, xmldata is a 'const char *' and doesn't need to be freed.
Well, it points to data that is in an arg that's parsed/allocated by vshCommandParse, and later freed by vshCommandFree, so both are correct statements. I'll change it to something that makes us both happy :-)
Maybe: xmldata is a const char* alias into memory managed by other variables, so it doesn't need to be freed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 :|
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.
An example usage:
virsh net-update add-last ip-dhcp-host \
Missing $net between the 'net-update' and 'add-last' args.
--xml "<host mac='00:11:22:33:44:55' ip='192.168.122.45'/>" \ --live --config
If you like, you can instead put the xml into a file, and call like this:
virsh net-update add-last ip-dhcp-host \
Likewise (I stopped checking past this point). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump