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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list