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

[Laine Stump]

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.