On 01/30/2012 08:09 AM, Peter Krempa wrote:
This patch adds API to modify domain metadata for running and stopped domains. The api supports changing description, title as well as the newly added <metadata> element. The API has support for storing data in the metadata element using xml namespaces.
* include/libvirt/libvirt.h.in * src/libvirt_public.syms - add function headers - add enum to select metadata to operate on - export functions * src/libvirt.c - add public api implementation * src/driver.h - add driver support * src/remote/remote_driver.c * src/remote/remote_protocol.x - wire up the remote protocol * include/libvirt/virterror.h * src/util/virterror.c - add a new error message note that metadata for domain are missing --- Diff to v1: - lifted the length restriction and tweaked commands according to that - moved argument checks from driver to main library implementation - sorted entries in symbol list
include/libvirt/libvirt.h.in | 24 ++++++ include/libvirt/virterror.h | 1 + src/driver.h | 16 ++++ src/libvirt.c | 181 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + src/remote/remote_driver.c | 2 + src/remote/remote_protocol.x | 24 ++++++-
'yum install dwarves' then 'make check' fails, because you didn't update src/remote_protocol-structs.
/** + * virDomainSetMetadata: + * @domain: a domain object + * @type: type of description, from virDomainMetadataType + * @metadata: new metadata text + * @key: XML namespace key, or NULL + * @uri: XML namespace URI, or NULL + * @flags: bitwise-OR of virDomainModificationImpact + * + * Sets the appropriate domain element given by @type to the + * value of @description. A @type of VIR_DOMAIN_METADATA_DESCRIPTION + * is free-form text; VIR_DOMAIN_METADATA_TITLE is free-form, but no + * newlines are permitted, and should be short (although the length is + * not enforced). For these two options @key and @uri are irrelevant and + * can be set to NULL.
s/can be/must be/ (since you error out if they are non-NULL)
+ * + * For type VIR_DOMAIN_METADATA_ELEMENT @metadata must be well-formed + * XML belonging to namespace defined by @uri with local name @key. + * + * Passing NULL for @metadata says to remove that element from the + * domain XML (passing the empty string leaves the element present). + * + * The resulting metadata will be present in virDomainGetXMLDesc(), + * as well as quick access through virDomainGetMetadata(). + * + * @flags controls whether the live domain, persistent configuration, + * or both will be modified. + * + * Returns 0 on success, -1 in case of failure. + */ +int +virDomainSetMetadata(virDomainPtr domain, + int type, + const char *metadata, + const char *key, + const char *uri, + unsigned int flags)
I think we've settled on a good API.
+ + switch (type) { + case VIR_DOMAIN_METADATA_TITLE: + if (metadata && strchr(metadata, '\n')) { + virLibDomainError(VIR_ERR_INVALID_ARG, "%s", + _("Domain title can't contain newlines")); + goto error; + } + /* fallthrough */ + case VIR_DOMAIN_METADATA_DESCRIPTION: + if (uri || key) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + break; + case VIR_DOMAIN_METADATA_ELEMENT: + if (!uri || !key) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + break;
These are okay,
+ default: + virLibDomainError(VIR_ERR_INVALID_ARG, "%s", + _("Invalid metadata type")); + goto error; + break;
but I don't like this one. If you have an older client trying to talk to a newer server, where the newer server understands a fourth type of metadata, then we should not be getting in the way of the client passing that data over RPC.
+/** + * virDomainGetMetadata: + * @domain: a domain object + * @type: type of description, from virDomainMetadataType + * @uri: XML namespace identifier + * @flags: bitwise-OR of virDomainModificationImpact + * + * Retrieves the appropriate domain element given by @type. + * If VIR_DOMAIN_METADATA_ELEMENT is requested parameter @uri + * must be set to the name of the namespace the requested elements + * belong to.
Also mention: Otherwise, uri must be NULL.
+ * + * If an element of the domain XML is not present, the resulting + * error will be VIR_ERR_NO_DOMAIN_METADATA. This method forms + * a shortcut for seeing information from virDomainSetMetadata() + * without having to go through virDomainGetXMLDesc(). + * + * @flags controls whether the live domain or persistent + * configuration will be queried. + * + * Returns the metadata string on success (caller must free), + * or NULL in case of failure. + */ +char * +virDomainGetMetadata(virDomainPtr domain, + int type, + const char *uri, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "type=%d, uri=%p, flags=%x", + type, uri, flags);
Here (and in the setter), I'd use "uri=%s"/NULLSTR(uri), rather "uri=%p"/uri, as that makes debugging a bit nicer when we know we have a validly-null string.
+ + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + goto error; + } + + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && + (flags & VIR_DOMAIN_AFFECT_CONFIG)) { + virLibDomainError(VIR_ERR_INVALID_ARG, "%s", + _("Can't specify both LIVE and CONFIG flags")); + goto error;
Elsewhere, we just used the terser: if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; }
+ } + + switch (type) { + case VIR_DOMAIN_METADATA_TITLE: + case VIR_DOMAIN_METADATA_DESCRIPTION: + if (uri) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + break; + case VIR_DOMAIN_METADATA_ELEMENT: + if (!uri) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + break;
Again, fine with these, (we have a defined semantic for these flags)
+ default: + virLibDomainError(VIR_ERR_INVALID_ARG, "%s", + _("Invalid metadata type")); + goto error; + break;
but not this (this prevents expansion).
--- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1114,6 +1114,26 @@ struct remote_domain_set_autostart_args { int autostart; };
+struct remote_domain_set_metadata_args { + remote_nonnull_domain dom; + int type; + remote_string metadata; + remote_string key; + remote_string uri; + unsigned int flags; +}; + +struct remote_domain_get_metadata_args { + remote_nonnull_domain dom; + int type; + remote_string uri; + unsigned int flags; +}; + +struct remote_domain_get_metadata_ret { + remote_string metadata;
This should be remote_nonnull_string (errors are returned automatically; the _ret struct is only used on success, which means it will never be used for NULL).
@@ -2708,7 +2728,9 @@ enum remote_procedure { REMOTE_PROC_STORAGE_VOL_RESIZE = 260, /* autogen autogen */
REMOTE_PROC_DOMAIN_PM_SUSPEND_FOR_DURATION = 261, /* autogen autogen */ - REMOTE_PROC_DOMAIN_GET_CPU_STATS = 262 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_GET_CPU_STATS = 262, /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_SET_METADATA = 263, /* autogen autogen */ + REMOTE_PROC_DOMAIN_GET_METADATA = 264 /* autogen autogen */
I'm surprised our generator didn't complain about things - it should be requiring nonnull_string in *_ret (but that can be a patch for a different day). I'm interested in this API making it into 0.9.10, and I think you're close enough that you could push this after fixing the issues I pointed out above. ACK with above issues fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org