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

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120919/15b64b0e/attachment-0001.sig>