7:04 p.m.
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.