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/
> + {"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.
> + /* 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.
> + 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.
However, there's one major omission preventing me acking this: virsh.pod
needs to document the new command.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list