On 09/20/2012 12:55 PM, Eric Blake wrote:
> 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 :).
>
In the interest of getting maximum testing, I'm pushing this version
(fixed up as you suggested, and also adding another fix to avoid leaking
a virNetwork object when the requested file doesn't exist). If this
method is successful for net-update, it can possibly be added to the
other commands that take a file-containing-xml option (the only
difference between the commands is in the name of that option, and that
name is usually unspecified in commandlines anyway).
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list