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

On 09/20/2012 09:05 AM, 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 mynet add-last ip-dhcp-host \ > "<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 mynet add ip-dhcp-host /tmp/myxml.xml > --live --config > > (since an xml element must always start with "<", but a filename > rarely does, virsh just checks the first character of the supplied > "--xml" argument, and decides whether to use the text directly or as a > filename. In the rare event that someone really does have a filename > starting with "<", they can specify it as "./<xxxx".) I like this version the best. > + * "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-first, add-last (add), delete, or modify)")}, > + {"section", VSH_OT_DATA, VSH_OFLAG_REQ, > + N_("which section of network configuration to update")}, > + {"xml", VSH_OT_DATA, VSH_OFLAG_REQ, > + N_("name of file containing xml (or, if it starts with '<', the complete " > + "xml element itself) to add/modify, or to be matched for search")}, Two spaces after ')' looks fishy. > + > + /* The goal is to have a full xml element in the "xml" > + * string. This is provided in the --xml option, either directly > + * (detected by the first character being "<"), or indirectly by > + * supplying a filename (first character isn't "<") that contains > + * the desired xml. > + */ > + > + if (vshCommandOptString(cmd, "xml", &xml) < 0) { > + vshError(ctl, "%s", _("malformed or missing xml argument")); > + goto cleanup; > + } > + > + if (*xml != '<') { > + /* contents of xmldata is actually the name of a file that > + * contains the xml. > + */ > + if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlFromFile) < 0) > + return false; > + /* NB: the original xml is just a const char * that points > + * to a string owned by the Command object, and will be freed > + * by vshCommandFree. so it's safe to lose its pointer here. > + */ > + xml = xmlFromFile; > + } > + > + 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, xml, flags) < 0) { > + vshError(ctl, _("Failed to update network %s"), > + virNetworkGetName(network)); > + goto cleanup; > + } > + > + if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { If you want, this could be simplified to: 'if (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) { and likewise simplify these two bitwise ops to 'if (live)'. ACK (unless I get outvoted by other opinions :).