On 01/09/2012 07:29 AM, Osier Yang wrote:
> The command here is just to demo the effect of the API and for
> one wants to play with it, it's not much useful as a virsh command
> IMO, so it's not for final commit.
On the contrary, I think that every libvirt API should be exposed via
virsh commands, so this patch should be polished and included in the
final series (that is, add virsh.pod documentation). As is, providing
this command allows for unit tests that our API works, whether or not
anyone else finds another use for the virsh API.
> ---
> tools/virsh.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 50 insertions(+), 0 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index e4b812e..0b15c46 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -6050,6 +6050,54 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd)
> }
>
> /*
> + * "normalize-device-xml" command
> + */
> +static const vshCmdInfo info_normalize_device_xml[] = {
> + {"help", N_("normalize the incoming device XML")},
> + {"desc", N_("Output the normalized device XML.")},
> + {NULL, NULL}
> +};
> +
> +static const vshCmdOptDef opts_normalize_device_xml[] = {
> + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("Device XML file")},
Given my thoughts in 0/6, I think we also need an optional --type=...
parameter. By default, if --type is not specified, virsh should read
the given XML and guess a type itself (that is, --type=domain is
redundant if xmlfile started with <domain>); but there are cases where
it is a required argument (that is, a top-level <interface> in the
user's file could either match --type=interface for use in commands like
iface-define [using conf/interface_conf.c], or --type=domain-device for
use in commands like attach-device [using conf/domain_conf.c], with the
two uses having quite a different effect). And definitely support
whatever flags we come up with, such as --validate, as well as an
intelligent --inactive (which maps to VIR_DOMAIN_XML_INACTIVE if type is
domain or domain-device, and to VIR_INTERFACE_XML_INACTIVE if type is
interface).
> +static bool
> +cmdNormalizeDeviceXML(vshControl *ctl, const vshCmd *cmd)
> +{
> + virDomainPtr dom;
> + const char *xmlfile = NULL;
> + bool ret = false;
> + char *buffer = NULL;
> + char *output = NULL;
> +
> + if (!vshConnectionUsability(ctl, ctl->conn))
> + return false;
> +
> + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> + return false;
> +
> + if (vshCommandOptString(cmd, "file", &xmlfile) <= 0)
> + return false;
> +
> + if (virFileReadAll(xmlfile, VIRSH_MAX_XML_FILE, &buffer) < 0)
> + return false;
> +
> + output = virDomainNormalizeDeviceXML(dom, buffer, 0);
> + if (output) {
> + vshPrint(ctl, "%s", output);
> + VIR_FREE(output);
> + ret = true;
> + }
> +
> + VIR_FREE(buffer);
> + virDomainFree(dom);
'dom' shouldn't be needed if you make the API more generic.
> @@ -15812,6 +15860,8 @@ static const vshCmdDef domManagementCmds[] = {
> opts_migrate_setspeed, info_migrate_setspeed, 0},
> {"migrate-getspeed", cmdMigrateGetMaxSpeed,
> opts_migrate_getspeed, info_migrate_getspeed, 0},
> + {"normalize-device-xml", cmdNormalizeDeviceXML,
> + opts_normalize_device_xml, info_normalize_device_xml, 0},
I'd name it 'normalize-xml', and stick it under the "Host and
Hypervisor" section alongside 'capabilities' and such.
I'd also look at adding a 7/6, to make 'virsh edit' and friends learn
how to re-open the editor if normalization fails validation, rather than
completely discarding all the user's edits if the actual define API fails.
--
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