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

[Laine Stump]

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).