[libvirt] [PATCH 00/11] Misc prep work for ACL support
The patch series for supporting ACLs has grown to about 80 patches, which no one wants to see on the list in one posting :-) So here are 11 patches which do misc cleanup/prep work for the ACL support
From: "Daniel P. Berrange" <berrange@redhat.com> It is possible for $line to be undefined at first used, if the symfile doesn't have a section prefix (which is the case for auto-generated symfiles). Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/check-symsorting.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/check-symsorting.pl b/src/check-symsorting.pl index c523c34..f360dfb 100755 --- a/src/check-symsorting.pl +++ b/src/check-symsorting.pl @@ -11,7 +11,7 @@ my $lastgroup = undef; foreach my $symfile (@ARGV) { open SYMFILE, $symfile or die "cannot read $symfile: $!"; - my $line; + my $line = 0; my $groupfile = ""; my @group; -- 1.8.1.4
On 05/02/2013 02:03 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
It is possible for $line to be undefined at first used, if the symfile doesn't have a section prefix (which is the case for auto-generated symfiles).
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/check-symsorting.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK Jan
From: "Daniel P. Berrange" <berrange@redhat.com> The there various methods named "virXXXXSecurityContext", which are specific to SELinux. Rename them all to "virXXXXSELinuxContext". They will still raise errors at runtime if SELinux is not compiled in Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 4 ++-- src/rpc/virnetserverclient.c | 10 +++++----- src/rpc/virnetserverclient.h | 4 ++-- src/rpc/virnetsocket.c | 8 ++++---- src/rpc/virnetsocket.h | 4 ++-- src/util/viridentity.c | 2 +- src/util/viridentity.h | 2 +- 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7e1eeb..4335152 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -862,7 +862,7 @@ virNetServerClientGetFD; virNetServerClientGetIdentity; virNetServerClientGetPrivateData; virNetServerClientGetReadonly; -virNetServerClientGetSecurityContext; +virNetServerClientGetSELinuxContext; virNetServerClientGetUNIXIdentity; virNetServerClientImmediateClose; virNetServerClientInit; @@ -933,7 +933,7 @@ virNetSocketClose; virNetSocketDupFD; virNetSocketGetFD; virNetSocketGetPort; -virNetSocketGetSecurityContext; +virNetSocketGetSELinuxContext; virNetSocketGetUNIXIdentity; virNetSocketHasCachedData; virNetSocketHasPassFD; diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 58fb0b4..50015f7 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -699,7 +699,7 @@ virNetServerClientCreateIdentity(virNetServerClientPtr client) #endif if (client->sock && - virNetSocketGetSecurityContext(client->sock, &seccontext) < 0) + virNetSocketGetSELinuxContext(client->sock, &seccontext) < 0) goto cleanup; if (!(ret = virIdentityNew())) @@ -734,7 +734,7 @@ virNetServerClientCreateIdentity(virNetServerClientPtr client) goto error; if (seccontext && virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_SECURITY_CONTEXT, + VIR_IDENTITY_ATTR_SELINUX_CONTEXT, seccontext) < 0) goto error; @@ -769,14 +769,14 @@ virIdentityPtr virNetServerClientGetIdentity(virNetServerClientPtr client) } -int virNetServerClientGetSecurityContext(virNetServerClientPtr client, - char **context) +int virNetServerClientGetSELinuxContext(virNetServerClientPtr client, + char **context) { int ret = 0; *context = NULL; virObjectLock(client); if (client->sock) - ret = virNetSocketGetSecurityContext(client->sock, context); + ret = virNetSocketGetSELinuxContext(client->sock, context); virObjectUnlock(client); return ret; } diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index b3b8df0..8536994 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -101,8 +101,8 @@ bool virNetServerClientIsLocal(virNetServerClientPtr client); int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client, uid_t *uid, gid_t *gid, pid_t *pid); -int virNetServerClientGetSecurityContext(virNetServerClientPtr client, - char **context); +int virNetServerClientGetSELinuxContext(virNetServerClientPtr client, + char **context); virIdentityPtr virNetServerClientGetIdentity(virNetServerClientPtr client); diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c4fd9ee..5a831dd 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1161,8 +1161,8 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr sock ATTRIBUTE_UNUSED, #endif #ifdef WITH_SELINUX -int virNetSocketGetSecurityContext(virNetSocketPtr sock, - char **context) +int virNetSocketGetSELinuxContext(virNetSocketPtr sock, + char **context) { security_context_t seccon = NULL; int ret = -1; @@ -1192,8 +1192,8 @@ cleanup: return ret; } #else -int virNetSocketGetSecurityContext(virNetSocketPtr sock ATTRIBUTE_UNUSED, - char **context) +int virNetSocketGetSELinuxContext(virNetSocketPtr sock ATTRIBUTE_UNUSED, + char **context) { *context = NULL; return 0; diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index 7392c72..7bced14 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -114,8 +114,8 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, uid_t *uid, gid_t *gid, pid_t *pid); -int virNetSocketGetSecurityContext(virNetSocketPtr sock, - char **context); +int virNetSocketGetSELinuxContext(virNetSocketPtr sock, + char **context); int virNetSocketSetBlocking(virNetSocketPtr sock, bool blocking); diff --git a/src/util/viridentity.c b/src/util/viridentity.c index c9efd3f..840485a 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -173,7 +173,7 @@ virIdentityPtr virIdentityGetSystem(void) goto error; if (seccontext && virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_SECURITY_CONTEXT, + VIR_IDENTITY_ATTR_SELINUX_CONTEXT, seccontext) < 0) goto error; diff --git a/src/util/viridentity.h b/src/util/viridentity.h index 39ab20e..d7293be 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -33,7 +33,7 @@ typedef enum { VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, VIR_IDENTITY_ATTR_SASL_USER_NAME, VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, - VIR_IDENTITY_ATTR_SECURITY_CONTEXT, + VIR_IDENTITY_ATTR_SELINUX_CONTEXT, VIR_IDENTITY_ATTR_LAST, } virIdentityAttrType; -- 1.8.1.4
On 05/02/2013 02:03 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The there various methods named "virXXXXSecurityContext", which are specific to SELinux. Rename them all to "virXXXXSELinuxContext". They will still raise errors at runtime if SELinux is not compiled in
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 4 ++-- src/rpc/virnetserverclient.c | 10 +++++----- src/rpc/virnetserverclient.h | 4 ++-- src/rpc/virnetsocket.c | 8 ++++---- src/rpc/virnetsocket.h | 4 ++-- src/util/viridentity.c | 2 +- src/util/viridentity.h | 2 +- 7 files changed, 17 insertions(+), 17 deletions(-)
ACK Jan
On 05/02/2013 06:03 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The there various methods named "virXXXXSecurityContext",
s/The there/There are/
which are specific to SELinux. Rename them all to "virXXXXSELinuxContext". They will still raise errors at runtime if SELinux is not compiled in
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Since PIDs can be reused, polkit prefers to be given a (PID,start time) pair. If given a PID on its own, it will attempt to lookup the start time in /proc/pid/stat, though this is subject to races. It is safer if the client app resolves the PID start time itself, because as long as the app has the client socket open, the client PID won't be reused. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/remote.c | 12 +++-- src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 4 +- src/rpc/virnetserverclient.c | 28 +++++++++-- src/rpc/virnetserverclient.h | 3 +- src/rpc/virnetsocket.c | 23 ++++++--- src/rpc/virnetsocket.h | 3 +- src/util/viridentity.h | 1 + src/util/virprocess.c | 117 +++++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 3 ++ src/util/virstring.c | 11 ++++ src/util/virstring.h | 2 + 12 files changed, 191 insertions(+), 17 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index e5e3f2c..7eca878 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2370,6 +2370,7 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, uid_t callerUid; gid_t callerGid; pid_t callerPid; + unsigned long long timestamp; /* If the client is root then we want to bypass the * policykit auth to avoid root being denied if @@ -2377,7 +2378,7 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, */ if (auth == VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, - &callerPid) < 0) { + &callerPid, ×tamp) < 0) { /* Don't do anything on error - it'll be validated at next * phase of auth anyway */ virResetLastError(); @@ -2803,6 +2804,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, pid_t callerPid = -1; gid_t callerGid = -1; uid_t callerUid = -1; + unsigned long long timestamp; const char *action; int status = -1; char *ident = NULL; @@ -2828,7 +2830,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, - &callerPid) < 0) { + &callerPid, ×tamp) < 0) { goto authfail; } @@ -2836,7 +2838,11 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, (long long) callerPid, callerUid); virCommandAddArg(cmd, "--process"); - virCommandAddArgFormat(cmd, "%lld", (long long) callerPid); + if (timestamp != 0) { + virCommandAddArgFormat(cmd, "%lld,%llu", (long long) callerPid, timestamp); + } else { + virCommandAddArgFormat(cmd, "%lld", (long long) callerPid); + } virCommandAddArg(cmd, "--allow-user-interaction"); if (virAsprintf(&ident, "pid:%lld,uid:%d", diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4335152..89b65b5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1748,6 +1748,7 @@ virStorageFileResize; virStringArrayHasString; virStringFreeList; virStringJoin; +virStringListLength; virStringSplit; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 97e5d74..bf0ee81 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -782,6 +782,7 @@ virLockDaemonClientNew(virNetServerClientPtr client, virLockDaemonClientPtr priv; uid_t clientuid; gid_t clientgid; + unsigned long long timestamp; bool privileged = opaque != NULL; if (VIR_ALLOC(priv) < 0) { @@ -798,7 +799,8 @@ virLockDaemonClientNew(virNetServerClientPtr client, if (virNetServerClientGetUNIXIdentity(client, &clientuid, &clientgid, - &priv->clientPid) < 0) + &priv->clientPid, + ×tamp) < 0) goto error; VIR_DEBUG("New client pid %llu uid %llu", diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 50015f7..f196e27 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -634,12 +634,15 @@ bool virNetServerClientIsLocal(virNetServerClientPtr client) int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client, - uid_t *uid, gid_t *gid, pid_t *pid) + uid_t *uid, gid_t *gid, pid_t *pid, + unsigned long long *timestamp) { int ret = -1; virObjectLock(client); if (client->sock) - ret = virNetSocketGetUNIXIdentity(client->sock, uid, gid, pid); + ret = virNetSocketGetUNIXIdentity(client->sock, + uid, gid, pid, + timestamp); virObjectUnlock(client); return ret; } @@ -649,6 +652,7 @@ static virIdentityPtr virNetServerClientCreateIdentity(virNetServerClientPtr client) { char *processid = NULL; + char *processtime = NULL; char *username = NULL; char *groupname = NULL; #if WITH_SASL @@ -662,15 +666,23 @@ virNetServerClientCreateIdentity(virNetServerClientPtr client) gid_t gid; uid_t uid; pid_t pid; - if (virNetSocketGetUNIXIdentity(client->sock, &uid, &gid, &pid) < 0) + unsigned long long timestamp; + if (virNetSocketGetUNIXIdentity(client->sock, + &uid, &gid, &pid, + ×tamp) < 0) goto cleanup; if (!(username = virGetUserName(uid))) goto cleanup; if (!(groupname = virGetGroupName(gid))) goto cleanup; - if (virAsprintf(&processid, "%lld", - (long long)pid) < 0) { + if (virAsprintf(&processid, "%llu", + (unsigned long long)pid) < 0) { + virReportOOMError(); + goto cleanup; + } + if (virAsprintf(&processtime, "%llu", + timestamp) < 0) { virReportOOMError(); goto cleanup; } @@ -720,6 +732,11 @@ virNetServerClientCreateIdentity(virNetServerClientPtr client) VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, processid) < 0) goto error; + if (processtime && + virIdentitySetAttr(ret, + VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, + processtime) < 0) + goto error; #if HAVE_SASL if (saslname && virIdentitySetAttr(ret, @@ -742,6 +759,7 @@ cleanup: VIR_FREE(username); VIR_FREE(groupname); VIR_FREE(processid); + VIR_FREE(processtime); VIR_FREE(seccontext); #if HAVE_SASL VIR_FREE(saslname); diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index 8536994..8d0fd18 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -99,7 +99,8 @@ bool virNetServerClientIsSecure(virNetServerClientPtr client); bool virNetServerClientIsLocal(virNetServerClientPtr client); int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client, - uid_t *uid, gid_t *gid, pid_t *pid); + uid_t *uid, gid_t *gid, pid_t *pid, + unsigned long long *timestamp); int virNetServerClientGetSELinuxContext(virNetServerClientPtr client, char **context); diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 5a831dd..3cf9bf3 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1103,31 +1103,41 @@ int virNetSocketGetPort(virNetSocketPtr sock) int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, uid_t *uid, gid_t *gid, - pid_t *pid) + pid_t *pid, + unsigned long long *timestamp) { struct ucred cr; socklen_t cr_len = sizeof(cr); + int ret = -1; + virObjectLock(sock); if (getsockopt(sock->fd, SOL_SOCKET, SO_PEERCRED, &cr, &cr_len) < 0) { virReportSystemError(errno, "%s", _("Failed to get client socket identity")); - virObjectUnlock(sock); - return -1; + goto cleanup; } + if (virProcessGetStartTime(cr.pid, timestamp) < 0) + goto cleanup; + *pid = cr.pid; *uid = cr.uid; *gid = cr.gid; + ret = 0; + +cleanup: virObjectUnlock(sock); - return 0; + return ret; } #elif defined(LOCAL_PEERCRED) + int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, uid_t *uid, gid_t *gid, - pid_t *pid) + pid_t *pid, + unsigned long long *timestamp ATTRIBUTE_UNUSED) { struct xucred cr; socklen_t cr_len = sizeof(cr); @@ -1151,7 +1161,8 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, int virNetSocketGetUNIXIdentity(virNetSocketPtr sock ATTRIBUTE_UNUSED, uid_t *uid ATTRIBUTE_UNUSED, gid_t *gid ATTRIBUTE_UNUSED, - pid_t *pid ATTRIBUTE_UNUSED) + pid_t *pid ATTRIBUTE_UNUSED, + unsigned long long *timestamp ATTRIBUTE_UNUSED) { /* XXX Many more OS support UNIX socket credentials we could port to. See dbus ....*/ virReportSystemError(ENOSYS, "%s", diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index 7bced14..ea42081 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -113,7 +113,8 @@ int virNetSocketGetPort(virNetSocketPtr sock); int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, uid_t *uid, gid_t *gid, - pid_t *pid); + pid_t *pid, + unsigned long long *timestamp); int virNetSocketGetSELinuxContext(virNetSocketPtr sock, char **context); diff --git a/src/util/viridentity.h b/src/util/viridentity.h index d7293be..4bae8d6 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -31,6 +31,7 @@ typedef enum { VIR_IDENTITY_ATTR_UNIX_USER_NAME, VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, + VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, VIR_IDENTITY_ATTR_SASL_USER_NAME, VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, VIR_IDENTITY_ATTR_SELINUX_CONTEXT, diff --git a/src/util/virprocess.c b/src/util/virprocess.c index fb81805..0cd97ba 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -33,11 +33,19 @@ #endif #include <sched.h> +#ifdef __FreeBSD__ +# include <sys/param.h> +# include <sys/sysctl.h> +# include <sys/user.h> +#endif + +#include "viratomic.h" #include "virprocess.h" #include "virerror.h" #include "viralloc.h" #include "virfile.h" #include "virlog.h" +#include "virstring.h" #include "virutil.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -755,3 +763,112 @@ virProcessSetMaxFiles(pid_t pid ATTRIBUTE_UNUSED, unsigned int files) return -1; } #endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_NOFILE)) */ + +#ifdef __linux__ +/* + * Port of code from polkitunixprocess.c under terms + * of the LGPLv2+ + */ +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp) +{ + char *filename = NULL; + char *buf = NULL; + char *tmp; + int ret = -1; + int len; + char **tokens = NULL; + + if (virAsprintf(&filename, "/proc/%llu/stat", + (unsigned long long)pid) < 0) { + virReportOOMError(); + return -1; + } + + if ((len = virFileReadAll(filename, 1024, &buf)) < 0) + goto cleanup; + + /* start time is the token at index 19 after the '(process name)' entry - since only this + * field can contain the ')' character, search backwards for this to avoid malicious + * processes trying to fool us + */ + + if (!(tmp = strrchr(buf, ')'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot find start time in %s"), + filename); + goto cleanup; + } + tmp += 2; /* skip ') ' */ + if ((tmp - buf) >= len) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot find start time in %s"), + filename); + goto cleanup; + } + + tokens = virStringSplit(tmp, " ", 0); + + if (virStringListLength(tokens) < 20) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot find start time in %s"), + filename); + goto cleanup; + } + + if (virStrToLong_ull(tokens[19], + NULL, + 10, + timestamp) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse start time %s in %s"), + tokens[19], filename); + goto cleanup; + } + + ret = 0; + +cleanup: + virStringFreeList(tokens); + VIR_FREE(filename); + VIR_FREE(buf); + return ret; +} +#elif defined(__FreeBSD__) +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp) +{ + struct kinfo_proc p; + int mib[4]; + size_t len = 4; + + sysctlnametomib("kern.proc.pid", mib, &len); + + len = sizeof(struct kinfo_proc); + mib[3] = pid; + + if (sysctl(mib, 4, p, &len, NULL, 0) < 0) { + virReportSystemError(errno, "%s", + _("Unable to query process ID start time")); + return -1; + } + + *timestamp = (unsigned long long)p.ki_start.tv_sec; + + return 0; + +} +#else +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp) +{ + static int warned = 0; + if (virAtomicIntInc(&warned) == 1) { + VIR_WARN("Process start time of pid %llu not available on this platform", + (unsigned long long)pid); + warned = true; + } + *timestamp = 0; + return 0; +} +#endif diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 5dea334..9f77bc5 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -47,6 +47,9 @@ int virProcessGetAffinity(pid_t pid, virBitmapPtr *map, int maxcpu); +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp); + int virProcessGetNamespaces(pid_t pid, size_t *nfdlist, int **fdlist); diff --git a/src/util/virstring.c b/src/util/virstring.c index 122ebb8..67c1dbb 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -180,3 +180,14 @@ virStringArrayHasString(char **strings, const char *needle) return false; } + + +size_t virStringListLength(char **strings) +{ + size_t i = 0; + + while (strings && strings[i]) + i++; + + return i; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index 2ceadc6..8d1995a 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -37,4 +37,6 @@ void virStringFreeList(char **strings); bool virStringArrayHasString(char **strings, const char *needle); +size_t virStringListLength(char **strings); + #endif /* __VIR_STRING_H__ */ -- 1.8.1.4
On Thu, May 02, 2013 at 01:03:41PM +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Since PIDs can be reused, polkit prefers to be given a (PID,start time) pair. If given a PID on its own, it will attempt to lookup the start time in /proc/pid/stat, though this is subject to races.
It is safer if the client app resolves the PID start time itself, because as long as the app has the client socket open, the client PID won't be reused.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
+#elif defined(__FreeBSD__) +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp) +{ + struct kinfo_proc p; + int mib[4]; + size_t len = 4; + + sysctlnametomib("kern.proc.pid", mib, &len); + + len = sizeof(struct kinfo_proc); + mib[3] = pid; + + if (sysctl(mib, 4, p, &len, NULL, 0) < 0) { + virReportSystemError(errno, "%s", + _("Unable to query process ID start time")); + return -1; + } + + *timestamp = (unsigned long long)p.ki_start.tv_sec; + + return 0; + +} +#else
Note this BSD specific code block has not even been compile tested. It is just copied from the polkit codebase with minimal conversion to libvirt standards. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/02/2013 06:07 AM, Daniel P. Berrange wrote:
On Thu, May 02, 2013 at 01:03:41PM +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Since PIDs can be reused, polkit prefers to be given a (PID,start time) pair. If given a PID on its own, it will attempt to lookup the start time in /proc/pid/stat, though this is subject to races.
It is safer if the client app resolves the PID start time itself, because as long as the app has the client socket open, the client PID won't be reused.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
+#elif defined(__FreeBSD__) +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp)
Note this BSD specific code block has not even been compile tested. It is just copied from the polkit codebase with minimal conversion to libvirt standards.
I've compile-tested it on FreeBSD (well, I had to do some conflict resolution in virstring to get this patch to apply, so I hope that when you rebase, that you end up doing the same resolution - but the conflicts didn't affect the BSD section of virprocess.c). It didn't quite work: util/virprocess.c: In function 'virProcessGetStartTime': util/virprocess.c:850: error: incompatible type for argument 3 of 'sysctl' I think you want to pass &p, not p, as the third argument (at least, doing that cleared up compilation for me, although I don't know how to test that the compiled result actually worked). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, May 03, 2013 at 01:38:47PM -0600, Eric Blake wrote:
On 05/02/2013 06:07 AM, Daniel P. Berrange wrote:
On Thu, May 02, 2013 at 01:03:41PM +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Since PIDs can be reused, polkit prefers to be given a (PID,start time) pair. If given a PID on its own, it will attempt to lookup the start time in /proc/pid/stat, though this is subject to races.
It is safer if the client app resolves the PID start time itself, because as long as the app has the client socket open, the client PID won't be reused.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
+#elif defined(__FreeBSD__) +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp)
Note this BSD specific code block has not even been compile tested. It is just copied from the polkit codebase with minimal conversion to libvirt standards.
I've compile-tested it on FreeBSD (well, I had to do some conflict resolution in virstring to get this patch to apply, so I hope that when you rebase, that you end up doing the same resolution - but the conflicts didn't affect the BSD section of virprocess.c). It didn't quite work:
util/virprocess.c: In function 'virProcessGetStartTime': util/virprocess.c:850: error: incompatible type for argument 3 of 'sysctl'
I think you want to pass &p, not p, as the third argument (at least, doing that cleared up compilation for me, although I don't know how to test that the compiled result actually worked).
Yes, you are corrrect - I mis-copied that Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/02/2013 06:03 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Since PIDs can be reused, polkit prefers to be given a (PID,start time) pair. If given a PID on its own, it will attempt to lookup the start time in /proc/pid/stat, though this is subject to races.
It is safer if the client app resolves the PID start time itself, because as long as the app has the client socket open, the client PID won't be reused.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/remote.c | 12 +++-- src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 4 +- src/rpc/virnetserverclient.c | 28 +++++++++-- src/rpc/virnetserverclient.h | 3 +- src/rpc/virnetsocket.c | 23 ++++++--- src/rpc/virnetsocket.h | 3 +- src/util/viridentity.h | 1 + src/util/virprocess.c | 117 +++++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 3 ++ src/util/virstring.c | 11 ++++ src/util/virstring.h | 2 + 12 files changed, 191 insertions(+), 17 deletions(-)
+int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp) +{ + char *filename = NULL; + char *buf = NULL; + char *tmp; + int ret = -1; + int len; + char **tokens = NULL; + + if (virAsprintf(&filename, "/proc/%llu/stat", + (unsigned long long)pid) < 0) { + virReportOOMError(); + return -1; + } + + if ((len = virFileReadAll(filename, 1024, &buf)) < 0) + goto cleanup; + + /* start time is the token at index 19 after the '(process name)' entry - since only this + * field can contain the ')' character, search backwards for this to avoid malicious + * processes trying to fool us + */
Talk about an arcane interface with the kernel. But the code looks correct; and more importantly, it picks the same number as polkit picks (since you copied polkit's code), no matter whether that number is a timestamp or something else. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Currently the virGetHostname() API has a bogus virConnectPtr parameter. This is because virtualization drivers directly reference this API in their virDriverPtr tables, tieing its API design to the public virConnectGetHostname API design. This also causes problems for access control checks since these must only be done for invocations from the public API, not internal invocation. Remove the bogus virConnectPtr parameter, and make each hypervisor driver provide a dedicated function for the driver API impl. This will allow access control checks to be easily inserted later. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/libvirtd-config.c | 2 +- src/check-driverimpls.pl | 1 - src/libxl/libxl_driver.c | 9 ++++++++- src/lxc/lxc_driver.c | 9 ++++++++- src/openvz/openvz_driver.c | 9 ++++++++- src/parallels/parallels_driver.c | 9 ++++++++- src/qemu/qemu_driver.c | 9 ++++++++- src/qemu/qemu_migration.c | 4 ++-- src/test/test_driver.c | 8 +++++++- src/uml/uml_driver.c | 9 ++++++++- src/util/virutil.c | 2 +- src/util/virutil.h | 2 +- src/vbox/vbox_tmpl.c | 9 ++++++++- src/xen/xen_driver.c | 9 ++++++++- src/xen/xend_internal.c | 4 ++-- 15 files changed, 78 insertions(+), 17 deletions(-) diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c index 5e3ae21..bebd826 100644 --- a/daemon/libvirtd-config.c +++ b/daemon/libvirtd-config.c @@ -290,7 +290,7 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) data->keepalive_count = 5; data->keepalive_required = 0; - localhost = virGetHostname(NULL); + localhost = virGetHostname(); if (localhost == NULL) { /* we couldn't resolve the hostname; assume that we are * running in disconnected operation, and report a less diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl index 2ba4864..52b14e4 100755 --- a/src/check-driverimpls.pl +++ b/src/check-driverimpls.pl @@ -44,7 +44,6 @@ while (<>) { # External impls next if $prefix eq "node"; - next if $prefix eq "vir"; if (defined $mainprefix) { if ($mainprefix ne $prefix) { diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 212d0fc..69dd1e4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1410,6 +1410,13 @@ libxlConnectGetVersion(virConnectPtr conn, unsigned long *version) return 0; } + +static char *libxlConnectGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return virGetHostname(); +} + + static int libxlConnectGetMaxVcpus(virConnectPtr conn, const char *type ATTRIBUTE_UNUSED) { @@ -4202,7 +4209,7 @@ static virDriver libxlDriver = { .connectClose = libxlConnectClose, /* 0.9.0 */ .connectGetType = libxlConnectGetType, /* 0.9.0 */ .connectGetVersion = libxlConnectGetVersion, /* 0.9.0 */ - .connectGetHostname = virGetHostname, /* 0.9.0 */ + .connectGetHostname = libxlConnectGetHostname, /* 0.9.0 */ .connectGetMaxVcpus = libxlConnectGetMaxVcpus, /* 0.9.0 */ .nodeGetInfo = libxlNodeGetInfo, /* 0.9.0 */ .connectGetCapabilities = libxlConnectGetCapabilities, /* 0.9.0 */ diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0becdc7..1475d04 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1562,6 +1562,13 @@ static int lxcConnectGetVersion(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned lo } +static char *lxcConnectGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return virGetHostname(); +} + + + /* * check whether the host supports CFS bandwidth * @@ -4402,7 +4409,7 @@ static virDriver lxcDriver = { .connectOpen = lxcConnectOpen, /* 0.4.2 */ .connectClose = lxcConnectClose, /* 0.4.2 */ .connectGetVersion = lxcConnectGetVersion, /* 0.4.6 */ - .connectGetHostname = virGetHostname, /* 0.6.3 */ + .connectGetHostname = lxcConnectGetHostname, /* 0.6.3 */ .connectGetSysinfo = lxcConnectGetSysinfo, /* 1.0.5 */ .nodeGetInfo = nodeGetInfo, /* 0.6.5 */ .connectGetCapabilities = lxcConnectGetCapabilities, /* 0.6.5 */ diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index f8bec62..9de36f4 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -343,6 +343,13 @@ static int openvzConnectGetVersion(virConnectPtr conn, unsigned long *version) { return 0; } + +static char *openvzConnectGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return virGetHostname(); +} + + static char *openvzDomainGetOSType(virDomainPtr dom) { struct openvz_driver *driver = dom->conn->privateData; @@ -2182,7 +2189,7 @@ static virDriver openvzDriver = { .connectClose = openvzConnectClose, /* 0.3.1 */ .connectGetType = openvzConnectGetType, /* 0.3.1 */ .connectGetVersion = openvzConnectGetVersion, /* 0.5.0 */ - .connectGetHostname = virGetHostname, /* 0.9.12 */ + .connectGetHostname = openvzConnectGetHostname, /* 0.9.12 */ .connectGetMaxVcpus = openvzConnectGetMaxVcpus, /* 0.4.6 */ .nodeGetInfo = nodeGetInfo, /* 0.3.2 */ .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.12 */ diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index e126967..f7e953a 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -1043,6 +1043,13 @@ cleanup: return ret; } + +static char *parallelsConnectGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return virGetHostname(); +} + + static int parallelsConnectListDomains(virConnectPtr conn, int *ids, int maxids) { @@ -2393,7 +2400,7 @@ static virDriver parallelsDriver = { .connectOpen = parallelsConnectOpen, /* 0.10.0 */ .connectClose = parallelsConnectClose, /* 0.10.0 */ .connectGetVersion = parallelsConnectGetVersion, /* 0.10.0 */ - .connectGetHostname = virGetHostname, /* 0.10.0 */ + .connectGetHostname = parallelsConnectGetHostname, /* 0.10.0 */ .nodeGetInfo = nodeGetInfo, /* 0.10.0 */ .connectGetCapabilities = parallelsConnectGetCapabilities, /* 0.10.0 */ .connectListDomains = parallelsConnectListDomains, /* 0.10.0 */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9492850..c42e7b7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1412,6 +1412,13 @@ cleanup: return ret; } + +static char *qemuConnectGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return virGetHostname(); +} + + static int qemuConnectListDomains(virConnectPtr conn, int *ids, int nids) { virQEMUDriverPtr driver = conn->privateData; int n; @@ -14651,7 +14658,7 @@ static virDriver qemuDriver = { .connectSupportsFeature = qemuConnectSupportsFeature, /* 0.5.0 */ .connectGetType = qemuConnectGetType, /* 0.2.0 */ .connectGetVersion = qemuConnectGetVersion, /* 0.2.0 */ - .connectGetHostname = virGetHostname, /* 0.3.3 */ + .connectGetHostname = qemuConnectGetHostname, /* 0.3.3 */ .connectGetSysinfo = qemuConnectGetSysinfo, /* 0.8.8 */ .connectGetMaxVcpus = qemuConnectGetMaxVcpus, /* 0.2.1 */ .nodeGetInfo = nodeGetInfo, /* 0.2.0 */ diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 6ad1c30..c0b6453 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -405,7 +405,7 @@ qemuMigrationCookieNew(virDomainObjPtr dom) goto no_memory; memcpy(mig->uuid, dom->def->uuid, VIR_UUID_BUFLEN); - if (!(mig->localHostname = virGetHostname(NULL))) + if (!(mig->localHostname = virGetHostname())) goto error; if (virGetHostUUID(mig->localHostuuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2373,7 +2373,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0; /* Get hostname */ - if ((hostname = virGetHostname(NULL)) == NULL) + if ((hostname = virGetHostname()) == NULL) goto cleanup; if (STRPREFIX(hostname, "localhost")) { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index d964fb2..26439e8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1223,6 +1223,12 @@ static int testConnectGetVersion(virConnectPtr conn ATTRIBUTE_UNUSED, return 0; } +static char *testConnectGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return virGetHostname(); +} + + static int testConnectIsSecure(virConnectPtr conn ATTRIBUTE_UNUSED) { return 1; @@ -5796,7 +5802,7 @@ static virDriver testDriver = { .connectOpen = testConnectOpen, /* 0.1.1 */ .connectClose = testConnectClose, /* 0.1.1 */ .connectGetVersion = testConnectGetVersion, /* 0.1.1 */ - .connectGetHostname = virGetHostname, /* 0.6.3 */ + .connectGetHostname = testConnectGetHostname, /* 0.6.3 */ .connectGetMaxVcpus = testConnectGetMaxVcpus, /* 0.3.2 */ .nodeGetInfo = testNodeGetInfo, /* 0.1.1 */ .connectGetCapabilities = testConnectGetCapabilities, /* 0.2.1 */ diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 9b0aba6..55cf204 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1494,6 +1494,13 @@ cleanup: return ret; } + +static char *umlConnectGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return virGetHostname(); +} + + static int umlConnectListDomains(virConnectPtr conn, int *ids, int nids) { struct uml_driver *driver = conn->privateData; int n; @@ -2597,7 +2604,7 @@ static virDriver umlDriver = { .connectClose = umlConnectClose, /* 0.5.0 */ .connectGetType = umlConnectGetType, /* 0.5.0 */ .connectGetVersion = umlConnectGetVersion, /* 0.5.0 */ - .connectGetHostname = virGetHostname, /* 0.5.0 */ + .connectGetHostname = umlConnectGetHostname, /* 0.5.0 */ .nodeGetInfo = nodeGetInfo, /* 0.5.0 */ .connectGetCapabilities = umlConnectGetCapabilities, /* 0.5.0 */ .connectListDomains = umlConnectListDomains, /* 0.5.0 */ diff --git a/src/util/virutil.c b/src/util/virutil.c index b9de33c..879b0b4 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2273,7 +2273,7 @@ char *virIndexToDiskName(int idx, const char *prefix) * we got from getaddrinfo(). Return the value from gethostname() * and hope for the best. */ -char *virGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +char *virGetHostname(void) { int r; char hostname[HOST_NAME_MAX+1], *result; diff --git a/src/util/virutil.h b/src/util/virutil.h index 39033db..341c734 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -260,7 +260,7 @@ static inline int geteuid (void) { return 0; } static inline int getgid (void) { return 0; } # endif -char *virGetHostname(virConnectPtr conn); +char *virGetHostname(void); char *virGetUserDirectory(void); char *virGetUserConfigDirectory(void); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 315ba9c..6112dd9 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -1089,6 +1089,13 @@ static int vboxConnectGetVersion(virConnectPtr conn, unsigned long *version) { return 0; } + +static char *vboxConnectGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return virGetHostname(); +} + + static int vboxConnectIsSecure(virConnectPtr conn ATTRIBUTE_UNUSED) { /* Driver is using local, non-network based transport */ return 1; @@ -9408,7 +9415,7 @@ virDriver NAME(Driver) = { .connectOpen = vboxConnectOpen, /* 0.6.3 */ .connectClose = vboxConnectClose, /* 0.6.3 */ .connectGetVersion = vboxConnectGetVersion, /* 0.6.3 */ - .connectGetHostname = virGetHostname, /* 0.6.3 */ + .connectGetHostname = vboxConnectGetHostname, /* 0.6.3 */ .connectGetMaxVcpus = vboxConnectGetMaxVcpus, /* 0.6.3 */ .nodeGetInfo = nodeGetInfo, /* 0.6.3 */ .connectGetCapabilities = vboxConnectGetCapabilities, /* 0.6.3 */ diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 0642edb..c6164b6 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -546,6 +546,13 @@ xenUnifiedConnectGetVersion(virConnectPtr conn, unsigned long *hvVer) return -1; } + +static char *xenUnifiedConnectGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return virGetHostname(); +} + + static int xenUnifiedConnectIsEncrypted(virConnectPtr conn ATTRIBUTE_UNUSED) { @@ -2353,7 +2360,7 @@ static virDriver xenUnifiedDriver = { .connectSupportsFeature = xenUnifiedConnectSupportsFeature, /* 0.3.2 */ .connectGetType = xenUnifiedConnectGetType, /* 0.0.3 */ .connectGetVersion = xenUnifiedConnectGetVersion, /* 0.0.3 */ - .connectGetHostname = virGetHostname, /* 0.7.3 */ + .connectGetHostname = xenUnifiedConnectGetHostname, /* 0.7.3 */ .connectGetMaxVcpus = xenUnifiedConnectGetMaxVcpus, /* 0.2.1 */ .nodeGetInfo = xenUnifiedNodeGetInfo, /* 0.1.0 */ .connectGetCapabilities = xenUnifiedConnectGetCapabilities, /* 0.2.1 */ diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 860bf11..e1f0708 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2926,7 +2926,7 @@ xenDaemonDomainSetAutostart(virDomainPtr domain, int autostart) } int -xenDaemonDomainMigratePrepare(virConnectPtr dconn, +xenDaemonDomainMigratePrepare(virConnectPtr dconn ATTRIBUTE_UNUSED, char **cookie ATTRIBUTE_UNUSED, int *cookielen ATTRIBUTE_UNUSED, const char *uri_in, @@ -2942,7 +2942,7 @@ xenDaemonDomainMigratePrepare(virConnectPtr dconn, * deallocates this string. */ if (uri_in == NULL) { - *uri_out = virGetHostname(dconn); + *uri_out = virGetHostname(); if (*uri_out == NULL) return -1; } -- 1.8.1.4
On 05/02/2013 02:03 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Currently the virGetHostname() API has a bogus virConnectPtr parameter. This is because virtualization drivers directly reference this API in their virDriverPtr tables, tieing its API design to the public virConnectGetHostname API design.
This also causes problems for access control checks since these must only be done for invocations from the public API, not internal invocation.
Remove the bogus virConnectPtr parameter, and make each hypervisor driver provide a dedicated function for the driver API impl. This will allow access control checks to be easily inserted later.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/libvirtd-config.c | 2 +- src/check-driverimpls.pl | 1 - src/libxl/libxl_driver.c | 9 ++++++++- src/lxc/lxc_driver.c | 9 ++++++++- src/openvz/openvz_driver.c | 9 ++++++++- src/parallels/parallels_driver.c | 9 ++++++++- src/qemu/qemu_driver.c | 9 ++++++++- src/qemu/qemu_migration.c | 4 ++-- src/test/test_driver.c | 8 +++++++- src/uml/uml_driver.c | 9 ++++++++- src/util/virutil.c | 2 +- src/util/virutil.h | 2 +- src/vbox/vbox_tmpl.c | 9 ++++++++- src/xen/xen_driver.c | 9 ++++++++- src/xen/xend_internal.c | 4 ++-- 15 files changed, 78 insertions(+), 17 deletions(-)
ACK Jan
From: "Daniel P. Berrange" <berrange@redhat.com> The individual hypervisor drivers were directly referencing APIs in src/nodeinfo.c in their virDriverPtr struct. Separate these methods, so there is always a wrapper in the hypervisor driver. This allows the unused virConnectPtr args to be removed from the nodeinfo.c file. Again this will ensure that ACL checks will only be performed on invocations that are directly associated with public API usage. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/check-driverimpls.pl | 3 -- src/lxc/lxc_driver.c | 92 +++++++++++++++++++++++++++++++++++---- src/nodeinfo.c | 42 ++++++++---------- src/nodeinfo.h | 22 ++++------ src/openvz/openvz_conf.c | 2 +- src/openvz/openvz_driver.c | 70 +++++++++++++++++++++++++++--- src/parallels/parallels_driver.c | 12 +++++- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_driver.c | 93 ++++++++++++++++++++++++++++++++++++---- src/uml/uml_driver.c | 92 +++++++++++++++++++++++++++++++++++---- src/vbox/vbox_tmpl.c | 30 +++++++++++-- src/xen/xen_driver.c | 24 ++++++++++- 12 files changed, 403 insertions(+), 81 deletions(-) diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl index 52b14e4..e385de0 100755 --- a/src/check-driverimpls.pl +++ b/src/check-driverimpls.pl @@ -42,9 +42,6 @@ while (<>) { my $prefix = $impl; $prefix =~ s/^([a-z]+(?:Unified)?)(.*?)$/$1/; - # External impls - next if $prefix eq "node"; - if (defined $mainprefix) { if ($mainprefix ne $prefix) { print "$ARGV:$. Bad prefix '$prefix' for API '$api', expecting '$mainprefix'\n"; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 1475d04..04de015 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4402,6 +4402,82 @@ lxcConnectGetSysinfo(virConnectPtr conn, unsigned int flags) } +static int +lxcNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, + virNodeInfoPtr nodeinfo) +{ + return nodeGetInfo(nodeinfo); +} + + +static int +lxcNodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cpuNum, + virNodeCPUStatsPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetCPUStats(cpuNum, params, nparams, flags); +} + + +static int +lxcNodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cellNum, + virNodeMemoryStatsPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetMemoryStats(cellNum, params, nparams, flags); +} + + +static int +lxcNodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned long long *freeMems, + int startCell, + int maxCells) +{ + return nodeGetCellsFreeMemory(freeMems, startCell, maxCells); +} + + +static unsigned long long +lxcNodeGetFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return nodeGetFreeMemory(); +} + + +static int +lxcNodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetMemoryParameters(params, nparams, flags); +} + + +static int +lxcNodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + return nodeSetMemoryParameters(params, nparams, flags); +} + + +static int +lxcNodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + return nodeGetCPUMap(cpumap, online, flags); +} + /* Function Tables */ static virDriver lxcDriver = { .no = VIR_DRV_LXC, @@ -4411,7 +4487,7 @@ static virDriver lxcDriver = { .connectGetVersion = lxcConnectGetVersion, /* 0.4.6 */ .connectGetHostname = lxcConnectGetHostname, /* 0.6.3 */ .connectGetSysinfo = lxcConnectGetSysinfo, /* 1.0.5 */ - .nodeGetInfo = nodeGetInfo, /* 0.6.5 */ + .nodeGetInfo = lxcNodeGetInfo, /* 0.6.5 */ .connectGetCapabilities = lxcConnectGetCapabilities, /* 0.6.5 */ .connectListDomains = lxcConnectListDomains, /* 0.4.2 */ .connectNumOfDomains = lxcConnectNumOfDomains, /* 0.4.2 */ @@ -4457,11 +4533,11 @@ static virDriver lxcDriver = { .domainSetSchedulerParameters = lxcDomainSetSchedulerParameters, /* 0.5.0 */ .domainSetSchedulerParametersFlags = lxcDomainSetSchedulerParametersFlags, /* 0.9.2 */ .domainInterfaceStats = lxcDomainInterfaceStats, /* 0.7.3 */ - .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.3 */ - .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.3 */ - .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.6.5 */ - .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.6.5 */ - .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ + .nodeGetCPUStats = lxcNodeGetCPUStats, /* 0.9.3 */ + .nodeGetMemoryStats = lxcNodeGetMemoryStats, /* 0.9.3 */ + .nodeGetCellsFreeMemory = lxcNodeGetCellsFreeMemory, /* 0.6.5 */ + .nodeGetFreeMemory = lxcNodeGetFreeMemory, /* 0.6.5 */ + .nodeGetCPUMap = lxcNodeGetCPUMap, /* 1.0.0 */ .connectDomainEventRegister = lxcConnectDomainEventRegister, /* 0.7.0 */ .connectDomainEventDeregister = lxcConnectDomainEventDeregister, /* 0.7.0 */ .connectIsEncrypted = lxcConnectIsEncrypted, /* 0.7.3 */ @@ -4474,8 +4550,8 @@ static virDriver lxcDriver = { .domainOpenConsole = lxcDomainOpenConsole, /* 0.8.6 */ .connectIsAlive = lxcConnectIsAlive, /* 0.9.8 */ .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ - .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */ - .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */ + .nodeGetMemoryParameters = lxcNodeGetMemoryParameters, /* 0.10.2 */ + .nodeSetMemoryParameters = lxcNodeSetMemoryParameters, /* 0.10.2 */ .domainSendProcessSignal = lxcDomainSendProcessSignal, /* 1.0.1 */ .domainShutdown = lxcDomainShutdown, /* 1.0.1 */ .domainShutdownFlags = lxcDomainShutdownFlags, /* 1.0.1 */ diff --git a/src/nodeinfo.c b/src/nodeinfo.c index a5b5608..f568a74 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -867,7 +867,7 @@ error: } #endif -int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) +int nodeGetInfo(virNodeInfoPtr nodeinfo) { virArch hostarch = virArchFromHost(); @@ -941,8 +941,7 @@ cleanup: #endif } -int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, - int cpuNum ATTRIBUTE_UNUSED, +int nodeGetCPUStats(int cpuNum ATTRIBUTE_UNUSED, virNodeCPUStatsPtr params ATTRIBUTE_UNUSED, int *nparams ATTRIBUTE_UNUSED, unsigned int flags) @@ -970,8 +969,7 @@ int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, #endif } -int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, - int cellNum ATTRIBUTE_UNUSED, +int nodeGetMemoryStats(int cellNum ATTRIBUTE_UNUSED, virNodeMemoryStatsPtr params ATTRIBUTE_UNUSED, int *nparams ATTRIBUTE_UNUSED, unsigned int flags) @@ -1192,8 +1190,7 @@ nodeMemoryParametersIsAllSupported(virTypedParameterPtr params, #endif int -nodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, - virTypedParameterPtr params ATTRIBUTE_UNUSED, +nodeSetMemoryParameters(virTypedParameterPtr params ATTRIBUTE_UNUSED, int nparams ATTRIBUTE_UNUSED, unsigned int flags) { @@ -1288,8 +1285,7 @@ cleanup: #define NODE_MEMORY_PARAMETERS_NUM 8 int -nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, - virTypedParameterPtr params ATTRIBUTE_UNUSED, +nodeGetMemoryParameters(virTypedParameterPtr params ATTRIBUTE_UNUSED, int *nparams ATTRIBUTE_UNUSED, unsigned int flags) { @@ -1436,8 +1432,7 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, } int -nodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED, - unsigned char **cpumap, +nodeGetCPUMap(unsigned char **cpumap, unsigned int *online, unsigned int flags) { @@ -1476,7 +1471,7 @@ nodeCapsInitNUMAFake(virCapsPtr caps ATTRIBUTE_UNUSED) int s, c, t; int id; - if (nodeGetInfo(NULL, &nodeinfo) < 0) + if (nodeGetInfo(&nodeinfo) < 0) return -1; ncpus = VIR_NODEINFO_MAXCPUS(nodeinfo); @@ -1518,8 +1513,7 @@ nodeCapsInitNUMAFake(virCapsPtr caps ATTRIBUTE_UNUSED) } static int -nodeGetCellsFreeMemoryFake(virConnectPtr conn ATTRIBUTE_UNUSED, - unsigned long long *freeMems, +nodeGetCellsFreeMemoryFake(unsigned long long *freeMems, int startCell, int maxCells ATTRIBUTE_UNUSED) { @@ -1544,7 +1538,7 @@ nodeGetCellsFreeMemoryFake(virConnectPtr conn ATTRIBUTE_UNUSED, } static unsigned long long -nodeGetFreeMemoryFake(virConnectPtr conn ATTRIBUTE_UNUSED) +nodeGetFreeMemoryFake(void) { double avail = physmem_available(); unsigned long long ret; @@ -1701,8 +1695,7 @@ cleanup: int -nodeGetCellsFreeMemory(virConnectPtr conn, - unsigned long long *freeMems, +nodeGetCellsFreeMemory(unsigned long long *freeMems, int startCell, int maxCells) { @@ -1711,7 +1704,7 @@ nodeGetCellsFreeMemory(virConnectPtr conn, int maxCell; if (numa_available() < 0) - return nodeGetCellsFreeMemoryFake(conn, freeMems, + return nodeGetCellsFreeMemoryFake(freeMems, startCell, maxCells); maxCell = numa_max_node(); @@ -1742,13 +1735,13 @@ cleanup: } unsigned long long -nodeGetFreeMemory(virConnectPtr conn) +nodeGetFreeMemory(void) { unsigned long long freeMem = 0; int n; if (numa_available() < 0) - return nodeGetFreeMemoryFake(conn); + return nodeGetFreeMemoryFake(); for (n = 0 ; n <= numa_max_node() ; n++) { @@ -1812,17 +1805,16 @@ int nodeCapsInitNUMA(virCapsPtr caps) { return nodeCapsInitNUMAFake(caps); } -int nodeGetCellsFreeMemory(virConnectPtr conn, - unsigned long long *freeMems, +int nodeGetCellsFreeMemory(unsigned long long *freeMems, int startCell, int maxCells) { - return nodeGetCellsFreeMemoryFake(conn, freeMems, + return nodeGetCellsFreeMemoryFake(freeMems, startCell, maxCells); } -unsigned long long nodeGetFreeMemory(virConnectPtr conn) +unsigned long long nodeGetFreeMemory(void) { - return nodeGetFreeMemoryFake(conn); + return nodeGetFreeMemoryFake(); } #endif diff --git a/src/nodeinfo.h b/src/nodeinfo.h index b0e5fbc..413fddd 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -26,40 +26,34 @@ # include "capabilities.h" -int nodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo); +int nodeGetInfo(virNodeInfoPtr nodeinfo); int nodeCapsInitNUMA(virCapsPtr caps); -int nodeGetCPUStats(virConnectPtr conn, - int cpuNum, +int nodeGetCPUStats(int cpuNum, virNodeCPUStatsPtr params, int *nparams, unsigned int flags); -int nodeGetMemoryStats(virConnectPtr conn, - int cellNum, +int nodeGetMemoryStats(int cellNum, virNodeMemoryStatsPtr params, int *nparams, unsigned int flags); -int nodeGetCellsFreeMemory(virConnectPtr conn, - unsigned long long *freeMems, +int nodeGetCellsFreeMemory(unsigned long long *freeMems, int startCell, int maxCells); -unsigned long long nodeGetFreeMemory(virConnectPtr conn); +unsigned long long nodeGetFreeMemory(void); virBitmapPtr nodeGetCPUBitmap(int *max_id); int nodeGetCPUCount(void); -int nodeGetMemoryParameters(virConnectPtr conn, - virTypedParameterPtr params, +int nodeGetMemoryParameters(virTypedParameterPtr params, int *nparams, unsigned int flags); -int nodeSetMemoryParameters(virConnectPtr conn, - virTypedParameterPtr params, +int nodeSetMemoryParameters(virTypedParameterPtr params, int nparams, unsigned int flags); -int nodeGetCPUMap(virConnectPtr conn, - unsigned char **cpumap, +int nodeGetCPUMap(unsigned char **cpumap, unsigned int *online, unsigned int flags); diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index a6f96c7..f176c2d 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -684,7 +684,7 @@ openvzGetNodeCPUs(void) { virNodeInfo nodeinfo; - if (nodeGetInfo(NULL, &nodeinfo) < 0) + if (nodeGetInfo(&nodeinfo) < 0) return 0; return nodeinfo.cpus; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 9de36f4..a721472 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -2182,6 +2182,64 @@ openvzConnectListAllDomains(virConnectPtr conn, } + +static int +openvzNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, + virNodeInfoPtr nodeinfo) +{ + return nodeGetInfo(nodeinfo); +} + + +static int +openvzNodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cpuNum, + virNodeCPUStatsPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetCPUStats(cpuNum, params, nparams, flags); +} + + +static int +openvzNodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cellNum, + virNodeMemoryStatsPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetMemoryStats(cellNum, params, nparams, flags); +} + + +static int +openvzNodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned long long *freeMems, + int startCell, + int maxCells) +{ + return nodeGetCellsFreeMemory(freeMems, startCell, maxCells); +} + + +static unsigned long long +openvzNodeGetFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return nodeGetFreeMemory(); +} + + +static int +openvzNodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + return nodeGetCPUMap(cpumap, online, flags); +} + + static virDriver openvzDriver = { .no = VIR_DRV_OPENVZ, .name = "OPENVZ", @@ -2191,12 +2249,12 @@ static virDriver openvzDriver = { .connectGetVersion = openvzConnectGetVersion, /* 0.5.0 */ .connectGetHostname = openvzConnectGetHostname, /* 0.9.12 */ .connectGetMaxVcpus = openvzConnectGetMaxVcpus, /* 0.4.6 */ - .nodeGetInfo = nodeGetInfo, /* 0.3.2 */ - .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.12 */ - .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.12 */ - .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.9.12 */ - .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.9.12 */ - .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ + .nodeGetInfo = openvzNodeGetInfo, /* 0.3.2 */ + .nodeGetCPUStats = openvzNodeGetCPUStats, /* 0.9.12 */ + .nodeGetMemoryStats = openvzNodeGetMemoryStats, /* 0.9.12 */ + .nodeGetCellsFreeMemory = openvzNodeGetCellsFreeMemory, /* 0.9.12 */ + .nodeGetFreeMemory = openvzNodeGetFreeMemory, /* 0.9.12 */ + .nodeGetCPUMap = openvzNodeGetCPUMap, /* 1.0.0 */ .connectGetCapabilities = openvzConnectGetCapabilities, /* 0.4.6 */ .connectListDomains = openvzConnectListDomains, /* 0.3.1 */ .connectNumOfDomains = openvzConnectNumOfDomains, /* 0.3.1 */ diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index f7e953a..7a71e17 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -724,7 +724,7 @@ parallelsLoadDomain(parallelsConnPtr privconn, virJSONValuePtr jobj) if (STREQ(tmp, "unlimited")) { virNodeInfo nodeinfo; - if (nodeGetInfo(NULL, &nodeinfo) < 0) { + if (nodeGetInfo(&nodeinfo) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Can't get node info")); goto cleanup; @@ -2394,6 +2394,14 @@ parallelsDomainDefineXML(virConnectPtr conn, const char *xml) return ret; } +static int +parallelsNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, + virNodeInfoPtr nodeinfo) +{ + return nodeGetInfo(nodeinfo); +} + + static virDriver parallelsDriver = { .no = VIR_DRV_PARALLELS, .name = "Parallels", @@ -2401,7 +2409,7 @@ static virDriver parallelsDriver = { .connectClose = parallelsConnectClose, /* 0.10.0 */ .connectGetVersion = parallelsConnectGetVersion, /* 0.10.0 */ .connectGetHostname = parallelsConnectGetHostname, /* 0.10.0 */ - .nodeGetInfo = nodeGetInfo, /* 0.10.0 */ + .nodeGetInfo = parallelsNodeGetInfo, /* 0.10.0 */ .connectGetCapabilities = parallelsConnectGetCapabilities, /* 0.10.0 */ .connectListDomains = parallelsConnectListDomains, /* 0.10.0 */ .connectNumOfDomains = parallelsConnectNumOfDomains, /* 0.10.0 */ diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2acf535..4d99147 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -839,7 +839,7 @@ virQEMUCapsInitCPU(virCapsPtr caps, cpu->arch = arch; - if (nodeGetInfo(NULL, &nodeinfo)) + if (nodeGetInfo(&nodeinfo)) goto error; cpu->type = VIR_CPU_TYPE_HOST; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c42e7b7..8631666 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14650,6 +14650,83 @@ cleanup: return ret; } + +static int +qemuNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, + virNodeInfoPtr nodeinfo) +{ + return nodeGetInfo(nodeinfo); +} + + +static int +qemuNodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cpuNum, + virNodeCPUStatsPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetCPUStats(cpuNum, params, nparams, flags); +} + + +static int +qemuNodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cellNum, + virNodeMemoryStatsPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetMemoryStats(cellNum, params, nparams, flags); +} + + +static int +qemuNodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned long long *freeMems, + int startCell, + int maxCells) +{ + return nodeGetCellsFreeMemory(freeMems, startCell, maxCells); +} + + +static unsigned long long +qemuNodeGetFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return nodeGetFreeMemory(); +} + + +static int +qemuNodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetMemoryParameters(params, nparams, flags); +} + + +static int +qemuNodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + return nodeSetMemoryParameters(params, nparams, flags); +} + + +static int +qemuNodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + return nodeGetCPUMap(cpumap, online, flags); +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -14661,7 +14738,7 @@ static virDriver qemuDriver = { .connectGetHostname = qemuConnectGetHostname, /* 0.3.3 */ .connectGetSysinfo = qemuConnectGetSysinfo, /* 0.8.8 */ .connectGetMaxVcpus = qemuConnectGetMaxVcpus, /* 0.2.1 */ - .nodeGetInfo = nodeGetInfo, /* 0.2.0 */ + .nodeGetInfo = qemuNodeGetInfo, /* 0.2.0 */ .connectGetCapabilities = qemuConnectGetCapabilities, /* 0.2.1 */ .connectListDomains = qemuConnectListDomains, /* 0.2.0 */ .connectNumOfDomains = qemuConnectNumOfDomains, /* 0.2.0 */ @@ -14742,10 +14819,10 @@ static virDriver qemuDriver = { .domainBlockPeek = qemuDomainBlockPeek, /* 0.4.4 */ .domainMemoryPeek = qemuDomainMemoryPeek, /* 0.4.4 */ .domainGetBlockInfo = qemuDomainGetBlockInfo, /* 0.8.1 */ - .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.3 */ - .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.3 */ - .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.4.4 */ - .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.4.4 */ + .nodeGetCPUStats = qemuNodeGetCPUStats, /* 0.9.3 */ + .nodeGetMemoryStats = qemuNodeGetMemoryStats, /* 0.9.3 */ + .nodeGetCellsFreeMemory = qemuNodeGetCellsFreeMemory, /* 0.4.4 */ + .nodeGetFreeMemory = qemuNodeGetFreeMemory, /* 0.4.4 */ .connectDomainEventRegister = qemuConnectDomainEventRegister, /* 0.5.0 */ .connectDomainEventDeregister = qemuConnectDomainEventDeregister, /* 0.5.0 */ .domainMigratePrepare2 = qemuDomainMigratePrepare2, /* 0.5.0 */ @@ -14824,9 +14901,9 @@ static virDriver qemuDriver = { .domainPMSuspendForDuration = qemuDomainPMSuspendForDuration, /* 0.9.11 */ .domainPMWakeup = qemuDomainPMWakeup, /* 0.9.11 */ .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.11 */ - .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */ - .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */ - .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ + .nodeGetMemoryParameters = qemuNodeGetMemoryParameters, /* 0.10.2 */ + .nodeSetMemoryParameters = qemuNodeSetMemoryParameters, /* 0.10.2 */ + .nodeGetCPUMap = qemuNodeGetCPUMap, /* 1.0.0 */ .domainFSTrim = qemuDomainFSTrim, /* 1.0.1 */ .domainOpenChannel = qemuDomainOpenChannel, /* 1.0.2 */ }; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 55cf204..3658cf7 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2596,6 +2596,82 @@ static int umlConnectListAllDomains(virConnectPtr conn, } +static int +umlNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, + virNodeInfoPtr nodeinfo) +{ + return nodeGetInfo(nodeinfo); +} + + +static int +umlNodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cpuNum, + virNodeCPUStatsPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetCPUStats(cpuNum, params, nparams, flags); +} + + +static int +umlNodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cellNum, + virNodeMemoryStatsPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetMemoryStats(cellNum, params, nparams, flags); +} + + +static int +umlNodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned long long *freeMems, + int startCell, + int maxCells) +{ + return nodeGetCellsFreeMemory(freeMems, startCell, maxCells); +} + + +static unsigned long long +umlNodeGetFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return nodeGetFreeMemory(); +} + + +static int +umlNodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetMemoryParameters(params, nparams, flags); +} + + +static int +umlNodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + return nodeSetMemoryParameters(params, nparams, flags); +} + + +static int +umlNodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned char **cpumap, + unsigned int *online, + unsigned int flags) +{ + return nodeGetCPUMap(cpumap, online, flags); +} + static virDriver umlDriver = { .no = VIR_DRV_UML, @@ -2605,7 +2681,7 @@ static virDriver umlDriver = { .connectGetType = umlConnectGetType, /* 0.5.0 */ .connectGetVersion = umlConnectGetVersion, /* 0.5.0 */ .connectGetHostname = umlConnectGetHostname, /* 0.5.0 */ - .nodeGetInfo = nodeGetInfo, /* 0.5.0 */ + .nodeGetInfo = umlNodeGetInfo, /* 0.5.0 */ .connectGetCapabilities = umlConnectGetCapabilities, /* 0.5.0 */ .connectListDomains = umlConnectListDomains, /* 0.5.0 */ .connectNumOfDomains = umlConnectNumOfDomains, /* 0.5.0 */ @@ -2639,11 +2715,11 @@ static virDriver umlDriver = { .domainGetAutostart = umlDomainGetAutostart, /* 0.5.0 */ .domainSetAutostart = umlDomainSetAutostart, /* 0.5.0 */ .domainBlockPeek = umlDomainBlockPeek, /* 0.5.0 */ - .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.3 */ - .nodeGetMemoryStats = nodeGetMemoryStats, /* 0.9.3 */ - .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.5.0 */ - .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.5.0 */ - .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ + .nodeGetCPUStats = umlNodeGetCPUStats, /* 0.9.3 */ + .nodeGetMemoryStats = umlNodeGetMemoryStats, /* 0.9.3 */ + .nodeGetCellsFreeMemory = umlNodeGetCellsFreeMemory, /* 0.5.0 */ + .nodeGetFreeMemory = umlNodeGetFreeMemory, /* 0.5.0 */ + .nodeGetCPUMap = umlNodeGetCPUMap, /* 1.0.0 */ .connectDomainEventRegister = umlConnectDomainEventRegister, /* 0.9.4 */ .connectDomainEventDeregister = umlConnectDomainEventDeregister, /* 0.9.4 */ .connectIsEncrypted = umlConnectIsEncrypted, /* 0.7.3 */ @@ -2656,8 +2732,8 @@ static virDriver umlDriver = { .domainOpenConsole = umlDomainOpenConsole, /* 0.8.6 */ .connectIsAlive = umlConnectIsAlive, /* 0.9.8 */ .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ - .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */ - .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */ + .nodeGetMemoryParameters = umlNodeGetMemoryParameters, /* 0.10.2 */ + .nodeSetMemoryParameters = umlNodeSetMemoryParameters, /* 0.10.2 */ }; static virStateDriver umlStateDriver = { diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 6112dd9..fef81b4 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -9404,6 +9404,30 @@ no_memory: #undef MATCH +static int +vboxNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, + virNodeInfoPtr nodeinfo) +{ + return nodeGetInfo(nodeinfo); +} + + +static int +vboxNodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned long long *freeMems, + int startCell, + int maxCells) +{ + return nodeGetCellsFreeMemory(freeMems, startCell, maxCells); +} + + +static unsigned long long +vboxNodeGetFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return nodeGetFreeMemory(); +} + /** * Function Tables @@ -9417,7 +9441,7 @@ virDriver NAME(Driver) = { .connectGetVersion = vboxConnectGetVersion, /* 0.6.3 */ .connectGetHostname = vboxConnectGetHostname, /* 0.6.3 */ .connectGetMaxVcpus = vboxConnectGetMaxVcpus, /* 0.6.3 */ - .nodeGetInfo = nodeGetInfo, /* 0.6.3 */ + .nodeGetInfo = vboxNodeGetInfo, /* 0.6.3 */ .connectGetCapabilities = vboxConnectGetCapabilities, /* 0.6.3 */ .connectListDomains = vboxConnectListDomains, /* 0.6.3 */ .connectNumOfDomains = vboxConnectNumOfDomains, /* 0.6.3 */ @@ -9455,8 +9479,8 @@ virDriver NAME(Driver) = { .domainDetachDevice = vboxDomainDetachDevice, /* 0.6.3 */ .domainDetachDeviceFlags = vboxDomainDetachDeviceFlags, /* 0.7.7 */ .domainUpdateDeviceFlags = vboxDomainUpdateDeviceFlags, /* 0.8.0 */ - .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.6.5 */ - .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.6.5 */ + .nodeGetCellsFreeMemory = vboxNodeGetCellsFreeMemory, /* 0.6.5 */ + .nodeGetFreeMemory = vboxNodeGetFreeMemory, /* 0.6.5 */ #if VBOX_API_VERSION >= 4000 .domainScreenshot = vboxDomainScreenshot, /* 0.9.2 */ #endif diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index c6164b6..d4676c9 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2349,6 +2349,26 @@ cleanup: virDomainDefFree(def); return ret; } + +static int +xenUnifiedNodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + return nodeGetMemoryParameters(params, nparams, flags); +} + + +static int +xenUnifiedNodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + return nodeSetMemoryParameters(params, nparams, flags); +} + /*----- Register with libvirt.c, and initialize Xen drivers. -----*/ /* The interface which we export upwards to libvirt.c. */ @@ -2443,8 +2463,8 @@ static virDriver xenUnifiedDriver = { .domainOpenConsole = xenUnifiedDomainOpenConsole, /* 0.8.6 */ .connectIsAlive = xenUnifiedConnectIsAlive, /* 0.9.8 */ .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ - .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */ - .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */ + .nodeGetMemoryParameters = xenUnifiedNodeGetMemoryParameters, /* 0.10.2 */ + .nodeSetMemoryParameters = xenUnifiedNodeSetMemoryParameters, /* 0.10.2 */ }; /** -- 1.8.1.4
On Thu, May 02, 2013 at 01:03:43PM +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The individual hypervisor drivers were directly referencing APIs in src/nodeinfo.c in their virDriverPtr struct. Separate these methods, so there is always a wrapper in the hypervisor driver. This allows the unused virConnectPtr args to be removed from the nodeinfo.c file. Again this will ensure that ACL checks will only be performed on invocations that are directly associated with public API usage.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/check-driverimpls.pl | 3 -- src/lxc/lxc_driver.c | 92 +++++++++++++++++++++++++++++++++++---- src/nodeinfo.c | 42 ++++++++---------- src/nodeinfo.h | 22 ++++------ src/openvz/openvz_conf.c | 2 +- src/openvz/openvz_driver.c | 70 +++++++++++++++++++++++++++--- src/parallels/parallels_driver.c | 12 +++++- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_driver.c | 93 ++++++++++++++++++++++++++++++++++++---- src/uml/uml_driver.c | 92 +++++++++++++++++++++++++++++++++++---- src/vbox/vbox_tmpl.c | 30 +++++++++++-- src/xen/xen_driver.c | 24 ++++++++++- 12 files changed, 403 insertions(+), 81 deletions(-)
diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl index 52b14e4..e385de0 100755 --- a/src/check-driverimpls.pl +++ b/src/check-driverimpls.pl @@ -42,9 +42,6 @@ while (<>) { my $prefix = $impl; $prefix =~ s/^([a-z]+(?:Unified)?)(.*?)$/$1/;
- # External impls - next if $prefix eq "node"; - if (defined $mainprefix) { if ($mainprefix ne $prefix) { print "$ARGV:$. Bad prefix '$prefix' for API '$api', expecting '$mainprefix'\n";
Opps, this hunk belongs a few patches later. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/02/2013 02:03 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The individual hypervisor drivers were directly referencing APIs in src/nodeinfo.c in their virDriverPtr struct. Separate these methods, so there is always a wrapper in the hypervisor driver. This allows the unused virConnectPtr args to be removed from the nodeinfo.c file. Again this will ensure that ACL checks will only be performed on invocations that are directly associated with public API usage.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/check-driverimpls.pl | 3 -- src/lxc/lxc_driver.c | 92 +++++++++++++++++++++++++++++++++++---- src/nodeinfo.c | 42 ++++++++---------- src/nodeinfo.h | 22 ++++------ src/openvz/openvz_conf.c | 2 +- src/openvz/openvz_driver.c | 70 +++++++++++++++++++++++++++--- src/parallels/parallels_driver.c | 12 +++++- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_driver.c | 93 ++++++++++++++++++++++++++++++++++++---- src/uml/uml_driver.c | 92 +++++++++++++++++++++++++++++++++++---- src/vbox/vbox_tmpl.c | 30 +++++++++++-- src/xen/xen_driver.c | 24 ++++++++++- 12 files changed, 403 insertions(+), 81 deletions(-)
diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl index 52b14e4..e385de0 100755 --- a/src/check-driverimpls.pl +++ b/src/check-driverimpls.pl @@ -42,9 +42,6 @@ while (<>) { my $prefix = $impl; $prefix =~ s/^([a-z]+(?:Unified)?)(.*?)$/$1/;
- # External impls - next if $prefix eq "node"; - if (defined $mainprefix) { if ($mainprefix ne $prefix) { print "$ARGV:$. Bad prefix '$prefix' for API '$api', expecting '$mainprefix'\n"; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 1475d04..04de015 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4402,6 +4402,82 @@ lxcConnectGetSysinfo(virConnectPtr conn, unsigned int flags) }
+static int +lxcNodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, + virNodeInfoPtr nodeinfo) ^ indentation
+{ + return nodeGetInfo(nodeinfo); +} + +
ACK with the check-driverimpls.pl hunk removed. Jan
From: "Daniel P. Berrange" <berrange@redhat.com> The individual hypervisor drivers were directly referencing APIs in virnodesuspend.c in their virDriverPtr struct. Separate these methods, so there is always a wrapper in the hypervisor driver. This allows the unused virConnectPtr args to be removed from the virnodesuspend.c file. Again this will ensure that ACL checks will only be performed on invocations that are directly associated with public API usage. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 13 ++++++++++++- src/qemu/qemu_driver.c | 13 ++++++++++++- src/uml/uml_driver.c | 12 +++++++++++- src/util/virnodesuspend.c | 3 +-- src/util/virnodesuspend.h | 3 +-- src/xen/xen_driver.c | 13 ++++++++++++- 6 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 04de015..aafbbec 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4478,6 +4478,17 @@ lxcNodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED, return nodeGetCPUMap(cpumap, online, flags); } + +static int +lxcNodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int target, + unsigned long long duration, + unsigned int flags) +{ + return nodeSuspendForDuration(target, duration, flags); +} + + /* Function Tables */ static virDriver lxcDriver = { .no = VIR_DRV_LXC, @@ -4549,7 +4560,7 @@ static virDriver lxcDriver = { .connectDomainEventDeregisterAny = lxcConnectDomainEventDeregisterAny, /* 0.8.0 */ .domainOpenConsole = lxcDomainOpenConsole, /* 0.8.6 */ .connectIsAlive = lxcConnectIsAlive, /* 0.9.8 */ - .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ + .nodeSuspendForDuration = lxcNodeSuspendForDuration, /* 0.9.8 */ .nodeGetMemoryParameters = lxcNodeGetMemoryParameters, /* 0.10.2 */ .nodeSetMemoryParameters = lxcNodeSetMemoryParameters, /* 0.10.2 */ .domainSendProcessSignal = lxcDomainSendProcessSignal, /* 1.0.1 */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8631666..a23bc6c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14727,6 +14727,17 @@ qemuNodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED, return nodeGetCPUMap(cpumap, online, flags); } + +static int +qemuNodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int target, + unsigned long long duration, + unsigned int flags) +{ + return nodeSuspendForDuration(target, duration, flags); +} + + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -14888,7 +14899,7 @@ static virDriver qemuDriver = { .domainBlockRebase = qemuDomainBlockRebase, /* 0.9.10 */ .domainBlockCommit = qemuDomainBlockCommit, /* 1.0.0 */ .connectIsAlive = qemuConnectIsAlive, /* 0.9.8 */ - .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ + .nodeSuspendForDuration = qemuNodeSuspendForDuration, /* 0.9.8 */ .domainSetBlockIoTune = qemuDomainSetBlockIoTune, /* 0.9.8 */ .domainGetBlockIoTune = qemuDomainGetBlockIoTune, /* 0.9.8 */ .domainSetNumaParameters = qemuDomainSetNumaParameters, /* 0.9.9 */ diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 3658cf7..88de681 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2673,6 +2673,16 @@ umlNodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED, } +static int +umlNodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int target, + unsigned long long duration, + unsigned int flags) +{ + return nodeSuspendForDuration(target, duration, flags); +} + + static virDriver umlDriver = { .no = VIR_DRV_UML, .name = "UML", @@ -2731,7 +2741,7 @@ static virDriver umlDriver = { .connectDomainEventDeregisterAny = umlConnectDomainEventDeregisterAny, /* 0.9.4 */ .domainOpenConsole = umlDomainOpenConsole, /* 0.8.6 */ .connectIsAlive = umlConnectIsAlive, /* 0.9.8 */ - .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ + .nodeSuspendForDuration = umlNodeSuspendForDuration, /* 0.9.8 */ .nodeGetMemoryParameters = umlNodeGetMemoryParameters, /* 0.10.2 */ .nodeSetMemoryParameters = umlNodeSetMemoryParameters, /* 0.10.2 */ }; diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index df40ccd..72a17cd 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -165,8 +165,7 @@ static void virNodeSuspend(void *cmdString) * -1 if suspending the node is not supported, or if a previous suspend * operation is still in progress. */ -int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, - unsigned int target, +int nodeSuspendForDuration(unsigned int target, unsigned long long duration, unsigned int flags) { diff --git a/src/util/virnodesuspend.h b/src/util/virnodesuspend.h index ac54015..b5f67dd 100644 --- a/src/util/virnodesuspend.h +++ b/src/util/virnodesuspend.h @@ -25,8 +25,7 @@ # include "internal.h" -int nodeSuspendForDuration(virConnectPtr conn, - unsigned int target, +int nodeSuspendForDuration(unsigned int target, unsigned long long duration, unsigned int flags); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index d4676c9..0342523 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2369,6 +2369,17 @@ xenUnifiedNodeSetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED, return nodeSetMemoryParameters(params, nparams, flags); } + +static int +xenUnifiedNodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int target, + unsigned long long duration, + unsigned int flags) +{ + return nodeSuspendForDuration(target, duration, flags); +} + + /*----- Register with libvirt.c, and initialize Xen drivers. -----*/ /* The interface which we export upwards to libvirt.c. */ @@ -2462,7 +2473,7 @@ static virDriver xenUnifiedDriver = { .connectDomainEventDeregisterAny = xenUnifiedConnectDomainEventDeregisterAny, /* 0.8.0 */ .domainOpenConsole = xenUnifiedDomainOpenConsole, /* 0.8.6 */ .connectIsAlive = xenUnifiedConnectIsAlive, /* 0.9.8 */ - .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ + .nodeSuspendForDuration = xenUnifiedNodeSuspendForDuration, /* 0.9.8 */ .nodeGetMemoryParameters = xenUnifiedNodeGetMemoryParameters, /* 0.10.2 */ .nodeSetMemoryParameters = xenUnifiedNodeSetMemoryParameters, /* 0.10.2 */ }; -- 1.8.1.4
On 05/02/2013 02:03 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The individual hypervisor drivers were directly referencing APIs in virnodesuspend.c in their virDriverPtr struct. Separate these methods, so there is always a wrapper in the hypervisor driver. This allows the unused virConnectPtr args to be removed from the virnodesuspend.c file. Again this will ensure that ACL checks will only be performed on invocations that are directly associated with public API usage.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 13 ++++++++++++- src/qemu/qemu_driver.c | 13 ++++++++++++- src/uml/uml_driver.c | 12 +++++++++++- src/util/virnodesuspend.c | 3 +-- src/util/virnodesuspend.h | 3 +-- src/xen/xen_driver.c | 13 ++++++++++++- 6 files changed, 49 insertions(+), 8 deletions(-)
ACK Jan
From: "Daniel P. Berrange" <berrange@redhat.com> In renaming driver API implementations to match the public API naming scheme, a few cases in the node device driver were missed. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/node_device/node_device_driver.c | 6 +++--- src/node_device/node_device_driver.h | 6 +++--- src/node_device/node_device_hal.c | 18 +++++++++--------- src/node_device/node_device_udev.c | 20 ++++++++++---------- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index c9bd68b..bfc606c 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -183,9 +183,9 @@ nodeListDevices(virConnectPtr conn, } int -nodeListAllNodeDevices(virConnectPtr conn, - virNodeDevicePtr **devices, - unsigned int flags) +nodeConnectListAllNodeDevices(virConnectPtr conn, + virNodeDevicePtr **devices, + unsigned int flags) { virNodeDeviceDriverStatePtr driver = conn->nodeDevicePrivateData; int ret = -1; diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index b6c6f18..acf50cb 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -65,9 +65,9 @@ int detect_scsi_host_caps_linux(union _virNodeDevCapData *d); int nodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags); int nodeListDevices(virConnectPtr conn, const char *cap, char **const names, int maxnames, unsigned int flags); -int nodeListAllNodeDevices(virConnectPtr conn, - virNodeDevicePtr **devices, - unsigned int flags); +int nodeConnectListAllNodeDevices(virConnectPtr conn, + virNodeDevicePtr **devices, + unsigned int flags); virNodeDevicePtr nodeDeviceLookupByName(virConnectPtr conn, const char *name); virNodeDevicePtr nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, const char *wwnn, diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 63245a9..b64ac6b 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -590,9 +590,9 @@ static void device_prop_modified(LibHalContext *ctx ATTRIBUTE_UNUSED, } -static int nodeDeviceStateInitialize(bool privileged ATTRIBUTE_UNUSED, - virStateInhibitCallback callback ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) +static int nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) { LibHalContext *hal_ctx = NULL; char **udi = NULL; @@ -691,7 +691,7 @@ static int nodeDeviceStateInitialize(bool privileged ATTRIBUTE_UNUSED, } -static int nodeDeviceStateCleanup(void) +static int nodeStateCleanup(void) { if (driverState) { nodeDeviceLock(driverState); @@ -708,7 +708,7 @@ static int nodeDeviceStateCleanup(void) } -static int nodeDeviceStateReload(void) +static int nodeStateReload(void) { DBusError err; char **udi = NULL; @@ -767,7 +767,7 @@ static virNodeDeviceDriver halNodeDeviceDriver = { .nodeDeviceClose = nodeDeviceClose, /* 0.5.0 */ .nodeNumOfDevices = nodeNumOfDevices, /* 0.5.0 */ .nodeListDevices = nodeListDevices, /* 0.5.0 */ - .connectListAllNodeDevices = nodeListAllNodeDevices, /* 0.10.2 */ + .connectListAllNodeDevices = nodeConnectListAllNodeDevices, /* 0.10.2 */ .nodeDeviceLookupByName = nodeDeviceLookupByName, /* 0.5.0 */ .nodeDeviceLookupSCSIHostByWWN = nodeDeviceLookupSCSIHostByWWN, /* 1.0.2 */ .nodeDeviceGetXMLDesc = nodeDeviceGetXMLDesc, /* 0.5.0 */ @@ -781,9 +781,9 @@ static virNodeDeviceDriver halNodeDeviceDriver = { static virStateDriver halStateDriver = { .name = "HAL", - .stateInitialize = nodeDeviceStateInitialize, /* 0.5.0 */ - .stateCleanup = nodeDeviceStateCleanup, /* 0.5.0 */ - .stateReload = nodeDeviceStateReload, /* 0.5.0 */ + .stateInitialize = nodeStateInitialize, /* 0.5.0 */ + .stateCleanup = nodeStateCleanup, /* 0.5.0 */ + .stateReload = nodeStateReload, /* 0.5.0 */ }; int halNodeRegister(void) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f98841e..97c9b06 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1438,7 +1438,7 @@ out: } -static int nodeDeviceStateCleanup(void) +static int nodeStateCleanup(void) { int ret = 0; @@ -1650,9 +1650,9 @@ out: return ret; } -static int nodeDeviceStateInitialize(bool privileged ATTRIBUTE_UNUSED, - virStateInhibitCallback callback ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) +static int nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) { udevPrivate *priv = NULL; struct udev *udev = NULL; @@ -1759,13 +1759,13 @@ out_unlock: out: if (ret == -1) { - nodeDeviceStateCleanup(); + nodeStateCleanup(); } return ret; } -static int nodeDeviceStateReload(void) +static int nodeStateReload(void) { return 0; } @@ -1798,7 +1798,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceClose = nodeDeviceClose, /* 0.7.3 */ .nodeNumOfDevices = nodeNumOfDevices, /* 0.7.3 */ .nodeListDevices = nodeListDevices, /* 0.7.3 */ - .connectListAllNodeDevices = nodeListAllNodeDevices, /* 0.10.2 */ + .connectListAllNodeDevices = nodeConnectListAllNodeDevices, /* 0.10.2 */ .nodeDeviceLookupByName = nodeDeviceLookupByName, /* 0.7.3 */ .nodeDeviceLookupSCSIHostByWWN = nodeDeviceLookupSCSIHostByWWN, /* 1.0.2 */ .nodeDeviceGetXMLDesc = nodeDeviceGetXMLDesc, /* 0.7.3 */ @@ -1811,9 +1811,9 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { static virStateDriver udevStateDriver = { .name = "udev", - .stateInitialize = nodeDeviceStateInitialize, /* 0.7.3 */ - .stateCleanup = nodeDeviceStateCleanup, /* 0.7.3 */ - .stateReload = nodeDeviceStateReload, /* 0.7.3 */ + .stateInitialize = nodeStateInitialize, /* 0.7.3 */ + .stateCleanup = nodeStateCleanup, /* 0.7.3 */ + .stateReload = nodeStateReload, /* 0.7.3 */ }; int udevNodeRegister(void) -- 1.8.1.4
On 05/02/2013 02:03 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
In renaming driver API implementations to match the public API naming scheme, a few cases in the node device driver were missed.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/node_device/node_device_driver.c | 6 +++--- src/node_device/node_device_driver.h | 6 +++--- src/node_device/node_device_hal.c | 18 +++++++++--------- src/node_device/node_device_udev.c | 20 ++++++++++---------- 4 files changed, 25 insertions(+), 25 deletions(-)
ACK, with the check-driverimpls.pl change from 5/11. Jan
From: "Daniel P. Berrange" <berrange@redhat.com> Several APIs allow for custom XML to be passed in. This is checked for ABI stability, which will ensure the UUID is not being changed. There isn't validation that the name did not change though. This could allow renaming of guests via the backdoor, which in turn could allow for bypassing access control restrictions based on names. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 11 +++++++++++ src/qemu/qemu_migration.c | 6 ------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a8b5dfd..d945b74 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12560,6 +12560,17 @@ virDomainDefCheckABIStability(virDomainDefPtr src, return false; } + /* Not strictly ABI related, but we want to make sure domains + * don't get silently re-named through the backdoor when passing + * custom XML into various APIs, since this would create havoc + */ + if (STRNEQ(src->name, dst->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain name '%s' does not match source '%s'"), + dst->name, src->name); + return false; + } + if (src->mem.max_balloon != dst->mem.max_balloon) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain max memory %lld does not match source %lld"), diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c0b6453..ebd0382 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1941,12 +1941,6 @@ char *qemuMigrationBegin(virQEMUDriverPtr driver, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; - if (STRNEQ(def->name, vm->def->name)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("target domain name doesn't match source name")); - goto cleanup; - } - if (!virDomainDefCheckABIStability(vm->def, def)) goto cleanup; -- 1.8.1.4
On 05/02/2013 06:03 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Several APIs allow for custom XML to be passed in. This is checked for ABI stability, which will ensure the UUID is not being changed. There isn't validation that the name did not change though. This could allow renaming of guests via the backdoor, which in turn could allow for bypassing access control restrictions based on names.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 11 +++++++++++ src/qemu/qemu_migration.c | 6 ------ 2 files changed, 11 insertions(+), 6 deletions(-)
How does this interact with APIs like virDomainMigrate(), which intentionally has a dname parameter for renaming the domain on the destination? Or is the idea that in virDomainMigrate2(), if you want to rename the domain, you have to do so through the dname argument and NOT via the backdoor of the dxml argument? /me reads docs: ... The migration will fail * if @dxml would cause any guest-visible changes. Pass NULL * if no changes are needed to the XML between source and destination. * @dxml cannot be used to rename the domain during migration (use * @dname for that purpose). Domain name in @dxml must match the * original domain name. Ah, so it looks like we indeed expect dname to be the only supported way, and that it is applied after dxml is first checked for ABI compatibility.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a8b5dfd..d945b74 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12560,6 +12560,17 @@ virDomainDefCheckABIStability(virDomainDefPtr src, return false; }
+ /* Not strictly ABI related, but we want to make sure domains + * don't get silently re-named through the backdoor when passing + * custom XML into various APIs, since this would create havoc + */ + if (STRNEQ(src->name, dst->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain name '%s' does not match source '%s'"), + dst->name, src->name); + return false; + }
The code makes sense, but I'd feel better delaying my ack until getting confirmation that I'm correctly interpreting that rename is intentionally denied on 'virsh save/restore', and rename during migration is allowed only through the dname argument, after dxml is already validated against the pre-rename xml. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, May 03, 2013 at 10:27:41AM -0600, Eric Blake wrote:
On 05/02/2013 06:03 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Several APIs allow for custom XML to be passed in. This is checked for ABI stability, which will ensure the UUID is not being changed. There isn't validation that the name did not change though. This could allow renaming of guests via the backdoor, which in turn could allow for bypassing access control restrictions based on names.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 11 +++++++++++ src/qemu/qemu_migration.c | 6 ------ 2 files changed, 11 insertions(+), 6 deletions(-)
How does this interact with APIs like virDomainMigrate(), which intentionally has a dname parameter for renaming the domain on the destination? Or is the idea that in virDomainMigrate2(), if you want to rename the domain, you have to do so through the dname argument and NOT via the backdoor of the dxml argument?
Yep, you must use the @dname parameter.
/me reads docs:
... The migration will fail * if @dxml would cause any guest-visible changes. Pass NULL * if no changes are needed to the XML between source and destination. * @dxml cannot be used to rename the domain during migration (use * @dname for that purpose). Domain name in @dxml must match the * original domain name.
Ah, so it looks like we indeed expect dname to be the only supported way, and that it is applied after dxml is first checked for ABI compatibility.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a8b5dfd..d945b74 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12560,6 +12560,17 @@ virDomainDefCheckABIStability(virDomainDefPtr src, return false; }
+ /* Not strictly ABI related, but we want to make sure domains + * don't get silently re-named through the backdoor when passing + * custom XML into various APIs, since this would create havoc + */ + if (STRNEQ(src->name, dst->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain name '%s' does not match source '%s'"), + dst->name, src->name); + return false; + }
The code makes sense, but I'd feel better delaying my ack until getting confirmation that I'm correctly interpreting that rename is intentionally denied on 'virsh save/restore', and rename during migration is allowed only through the dname argument, after dxml is already validated against the pre-rename xml.
Yes, rename is intentionally denied in save/restore. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/03/2013 10:37 AM, Daniel P. Berrange wrote:
+ /* Not strictly ABI related, but we want to make sure domains + * don't get silently re-named through the backdoor when passing + * custom XML into various APIs, since this would create havoc + */ + if (STRNEQ(src->name, dst->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain name '%s' does not match source '%s'"), + dst->name, src->name); + return false; + }
The code makes sense, but I'd feel better delaying my ack until getting confirmation that I'm correctly interpreting that rename is intentionally denied on 'virsh save/restore', and rename during migration is allowed only through the dname argument, after dxml is already validated against the pre-rename xml.
Yes, rename is intentionally denied in save/restore.
Good, I got it right, and you have ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Currently the parsing of XML is pushed down into the various migration helper APIs. This makes it difficult to insert the correct access control checks, since one helper API services many public APIs. Pull the parsing of XML up to the top level of the QEMU driver APIs --- src/qemu/qemu_driver.c | 92 ++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_migration.c | 35 +++++------------- src/qemu/qemu_migration.h | 6 ++-- 3 files changed, 99 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a23bc6c..9abf6cb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9235,6 +9235,8 @@ qemuDomainMigratePrepareTunnel(virConnectPtr dconn, const char *dom_xml) { virQEMUDriverPtr driver = dconn->privateData; + virCapsPtr caps = NULL; + virDomainDefPtr def = NULL; int ret = -1; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); @@ -9262,11 +9264,30 @@ qemuDomainMigratePrepareTunnel(virConnectPtr dconn, goto cleanup; } + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(def = virDomainDefParseString(dom_xml, caps, driver->xmlopt, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (dname) { + VIR_FREE(def->name); + if (!(def->name = strdup(dname))) { + virReportOOMError(); + goto cleanup; + } + } + ret = qemuMigrationPrepareTunnel(driver, dconn, NULL, 0, NULL, NULL, /* No cookies in v2 */ - st, dname, dom_xml, flags); + st, def, flags); + def = NULL; cleanup: + virDomainDefFree(def); + virObjectUnref(caps); return ret; } @@ -9286,6 +9307,8 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, const char *dom_xml) { virQEMUDriverPtr driver = dconn->privateData; + virCapsPtr caps = NULL; + virDomainDefPtr def = NULL; int ret = -1; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); @@ -9314,6 +9337,22 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, goto cleanup; } + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(def = virDomainDefParseString(dom_xml, caps, driver->xmlopt, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (dname) { + VIR_FREE(def->name); + if (!(def->name = strdup(dname))) { + virReportOOMError(); + goto cleanup; + } + } + /* Do not use cookies in v2 protocol, since the cookie * length was not sufficiently large, causing failures * migrating between old & new libvirtd @@ -9321,9 +9360,12 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, ret = qemuMigrationPrepareDirect(driver, dconn, NULL, 0, NULL, NULL, /* No cookies */ uri_in, uri_out, - dname, dom_xml, flags); + def, flags); + def = NULL; cleanup: + virDomainDefFree(def); + virObjectUnref(caps); return ret; } @@ -9513,6 +9555,8 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, const char *dom_xml) { virQEMUDriverPtr driver = dconn->privateData; + virCapsPtr caps = NULL; + virDomainDefPtr def = NULL; int ret = -1; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); @@ -9534,13 +9578,32 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, goto cleanup; } + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(def = virDomainDefParseString(dom_xml, caps, driver->xmlopt, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (dname) { + VIR_FREE(def->name); + if (!(def->name = strdup(dname))) { + virReportOOMError(); + goto cleanup; + } + } + ret = qemuMigrationPrepareDirect(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, uri_in, uri_out, - dname, dom_xml, flags); + def, flags); + def = NULL; cleanup: + virDomainDefFree(def); + virObjectUnref(caps); return ret; } @@ -9558,6 +9621,8 @@ qemuDomainMigratePrepareTunnel3(virConnectPtr dconn, const char *dom_xml) { virQEMUDriverPtr driver = dconn->privateData; + virCapsPtr caps = NULL; + virDomainDefPtr def = NULL; int ret = -1; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); @@ -9578,12 +9643,31 @@ qemuDomainMigratePrepareTunnel3(virConnectPtr dconn, goto cleanup; } + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(def = virDomainDefParseString(dom_xml, caps, driver->xmlopt, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (dname) { + VIR_FREE(def->name); + if (!(def->name = strdup(dname))) { + virReportOOMError(); + goto cleanup; + } + } + ret = qemuMigrationPrepareTunnel(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, - st, dname, dom_xml, flags); + st, def, flags); + def = NULL; cleanup: + virDomainDefFree(def); + virObjectUnref(caps); return ret; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index ebd0382..8753c0d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1984,13 +1984,11 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, int cookieinlen, char **cookieout, int *cookieoutlen, - const char *dname, - const char *dom_xml, + virDomainDefPtr def, virStreamPtr st, unsigned int port, unsigned long flags) { - virDomainDefPtr def = NULL; virDomainObjPtr vm = NULL; virDomainEventPtr event = NULL; int ret = -1; @@ -2034,22 +2032,9 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; - if (!(def = virDomainDefParseString(dom_xml, caps, driver->xmlopt, - QEMU_EXPECTED_VIRT_TYPES, - VIR_DOMAIN_XML_INACTIVE))) - goto cleanup; - if (!qemuMigrationIsAllowed(driver, NULL, def, true)) goto cleanup; - /* Target domain name, maybe renamed. */ - if (dname) { - origname = def->name; - def->name = strdup(dname); - if (def->name == NULL) - goto cleanup; - } - /* Let migration hook filter domain XML */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { char *xml; @@ -2305,20 +2290,19 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver, char **cookieout, int *cookieoutlen, virStreamPtr st, - const char *dname, - const char *dom_xml, + virDomainDefPtr def, unsigned long flags) { int ret; VIR_DEBUG("driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, " - "cookieout=%p, cookieoutlen=%p, st=%p, dname=%s, dom_xml=%s " + "cookieout=%p, cookieoutlen=%p, st=%p, def=%p, " "flags=%lx", driver, dconn, NULLSTR(cookiein), cookieinlen, - cookieout, cookieoutlen, st, NULLSTR(dname), dom_xml, flags); + cookieout, cookieoutlen, st, def, flags); ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, - cookieout, cookieoutlen, dname, dom_xml, + cookieout, cookieoutlen, def, st, 0, flags); return ret; } @@ -2333,8 +2317,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, int *cookieoutlen, const char *uri_in, char **uri_out, - const char *dname, - const char *dom_xml, + virDomainDefPtr def, unsigned long flags) { static int port = 0; @@ -2347,10 +2330,10 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, VIR_DEBUG("driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, uri_in=%s, uri_out=%p, " - "dname=%s, dom_xml=%s flags=%lx", + "def=%p, flags=%lx", driver, dconn, NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, NULLSTR(uri_in), uri_out, - NULLSTR(dname), dom_xml, flags); + def, flags); /* The URI passed in may be NULL or a string "tcp://somehostname:port". * @@ -2445,7 +2428,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, VIR_DEBUG("Generated uri_out=%s", *uri_out); ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, - cookieout, cookieoutlen, dname, dom_xml, + cookieout, cookieoutlen, def, NULL, this_port, flags); cleanup: VIR_FREE(hostname); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 505e911..b42fe4e 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -98,8 +98,7 @@ int qemuMigrationPrepareTunnel(virQEMUDriverPtr driver, char **cookieout, int *cookieoutlen, virStreamPtr st, - const char *dname, - const char *dom_xml, + virDomainDefPtr def, unsigned long flags); int qemuMigrationPrepareDirect(virQEMUDriverPtr driver, @@ -110,8 +109,7 @@ int qemuMigrationPrepareDirect(virQEMUDriverPtr driver, int *cookieoutlen, const char *uri_in, char **uri_out, - const char *dname, - const char *dom_xml, + virDomainDefPtr def, unsigned long flags); int qemuMigrationPerform(virQEMUDriverPtr driver, -- 1.8.1.4
On 05/02/2013 06:03 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Currently the parsing of XML is pushed down into the various migration helper APIs. This makes it difficult to insert the correct access control checks, since one helper API services many public APIs. Pull the parsing of XML up to the top level of the QEMU driver APIs --- src/qemu/qemu_driver.c | 92 ++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_migration.c | 35 +++++------------- src/qemu/qemu_migration.h | 6 ++-- 3 files changed, 99 insertions(+), 34 deletions(-)
ACK.
+ if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(def = virDomainDefParseString(dom_xml, caps, driver->xmlopt, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (dname) { + VIR_FREE(def->name); + if (!(def->name = strdup(dname))) { + virReportOOMError(); + goto cleanup; + } + }
This section is repeated a bit; is it worth further factoring it into a helper function to be called from each site? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, May 03, 2013 at 10:30:19AM -0600, Eric Blake wrote:
On 05/02/2013 06:03 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Currently the parsing of XML is pushed down into the various migration helper APIs. This makes it difficult to insert the correct access control checks, since one helper API services many public APIs. Pull the parsing of XML up to the top level of the QEMU driver APIs --- src/qemu/qemu_driver.c | 92 ++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_migration.c | 35 +++++------------- src/qemu/qemu_migration.h | 6 ++-- 3 files changed, 99 insertions(+), 34 deletions(-)
ACK.
+ if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(def = virDomainDefParseString(dom_xml, caps, driver->xmlopt, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (dname) { + VIR_FREE(def->name); + if (!(def->name = strdup(dname))) { + virReportOOMError(); + goto cleanup; + } + }
This section is repeated a bit; is it worth further factoring it into a helper function to be called from each site?
In doing the refactoring I'm tending to prefer clarity of code even at the cost of some duplicated code, since it makes it easier to visibly see that the ACL check are being done at the correct time or place. Removing the special cases is also letting me do some automated analysis of ACL rules Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
From: "Daniel P. Berrange" <berrange@redhat.com> The LXC, QEMU, and LibXL drivers have all merged their handling of the attach/update/modify device APIs into one large 'xxxxDomainModifyDeviceFlags' which then does a 'switch()' based on the actual API being invoked. While this saves some lines of code, it is not really all that significant in the context of the driver API impls as a whole. This merger of the handling of different APIs creates pain when wanting to automated analysis of the code and do things which are specific to individual APIs. The slight duplication of code from unmerged the API impls, is preferrable to allow for easier automated analysis. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libxl/libxl_driver.c | 241 +++++++++++++++++++++++++++++------- src/lxc/lxc_driver.c | 278 ++++++++++++++++++++++++++++++++--------- src/qemu/qemu_driver.c | 316 ++++++++++++++++++++++++++++++++++++++--------- 3 files changed, 671 insertions(+), 164 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 69dd1e4..b5c7178 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3538,16 +3538,10 @@ cleanup: return ret; } -/* Actions for libxlDomainModifyDeviceFlags */ -enum { - LIBXL_DEVICE_ATTACH, - LIBXL_DEVICE_DETACH, - LIBXL_DEVICE_UPDATE, -}; static int -libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, - unsigned int flags, int action) +libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, + unsigned int flags) { libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; @@ -3600,23 +3594,11 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, driver->xmlopt))) goto cleanup; - switch (action) { - case LIBXL_DEVICE_ATTACH: - ret = libxlDomainAttachDeviceConfig(vmdef, dev); - break; - case LIBXL_DEVICE_DETACH: - ret = libxlDomainDetachDeviceConfig(vmdef, dev); - break; - case LIBXL_DEVICE_UPDATE: - ret = libxlDomainUpdateDeviceConfig(vmdef, dev); - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown domain modify action %d"), action); - break; - } - } else + if ((ret = libxlDomainAttachDeviceConfig(vmdef, dev)) < 0) + goto cleanup; + } else { ret = 0; + } if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { /* If dev exists it was created to modify the domain config. Free it. */ @@ -3626,21 +3608,9 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; - switch (action) { - case LIBXL_DEVICE_ATTACH: - ret = libxlDomainAttachDeviceLive(priv, vm, dev); - break; - case LIBXL_DEVICE_DETACH: - ret = libxlDomainDetachDeviceLive(priv, vm, dev); - break; - case LIBXL_DEVICE_UPDATE: - ret = libxlDomainUpdateDeviceLive(priv, vm, dev); - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown domain modify action %d"), action); - break; - } + if ((ret = libxlDomainAttachDeviceLive(priv, vm, dev)) < 0) + goto cleanup; + /* * update domain status forcibly because the domain status may be * changed even if we attach the device failed. @@ -3668,13 +3638,6 @@ cleanup: } static int -libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, - unsigned int flags) -{ - return libxlDomainModifyDeviceFlags(dom, xml, flags, LIBXL_DEVICE_ATTACH); -} - -static int libxlDomainAttachDevice(virDomainPtr dom, const char *xml) { return libxlDomainAttachDeviceFlags(dom, xml, @@ -3685,7 +3648,98 @@ static int libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - return libxlDomainModifyDeviceFlags(dom, xml, flags, LIBXL_DEVICE_DETACH); + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainDefPtr vmdef = NULL; + virDomainDeviceDefPtr dev = NULL; + libxlDomainObjPrivatePtr priv; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE | + VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1); + + libxlDriverLock(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); + + if (!vm) { + virReportError(VIR_ERR_NO_DOMAIN, "%s", _("no domain with matching uuid")); + goto cleanup; + } + + if (virDomainObjIsActive(vm)) { + if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + } else { + if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) + flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; + /* check consistency between flags and the vm state */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("Domain is not running")); + goto cleanup; + } + } + + if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot modify device on transient domain")); + goto cleanup; + } + + priv = vm->privateData; + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + if (!(dev = virDomainDeviceDefParse(xml, vm->def, + driver->caps, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + /* Make a copy for updated domain. */ + if (!(vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, + driver->xmlopt))) + goto cleanup; + + if ((ret = libxlDomainDetachDeviceConfig(vmdef, dev)) < 0) + goto cleanup; + } else { + ret = 0; + } + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + /* If dev exists it was created to modify the domain config. Free it. */ + virDomainDeviceDefFree(dev); + if (!(dev = virDomainDeviceDefParse(xml, vm->def, + driver->caps, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if ((ret = libxlDomainDetachDeviceLive(priv, vm, dev)) < 0) + goto cleanup; + + /* + * update domain status forcibly because the domain status may be + * changed even if we attach the device failed. + */ + if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) + ret = -1; + } + + /* Finally, if no error until here, we can save config. */ + if (!ret && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) { + ret = virDomainSaveConfig(driver->configDir, vmdef); + if (!ret) { + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; + } + } + +cleanup: + virDomainDefFree(vmdef); + virDomainDeviceDefFree(dev); + if (vm) + virObjectUnlock(vm); + libxlDriverUnlock(driver); + return ret; } static int @@ -3699,7 +3753,98 @@ static int libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - return libxlDomainModifyDeviceFlags(dom, xml, flags, LIBXL_DEVICE_UPDATE); + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainDefPtr vmdef = NULL; + virDomainDeviceDefPtr dev = NULL; + libxlDomainObjPrivatePtr priv; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE | + VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1); + + libxlDriverLock(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); + + if (!vm) { + virReportError(VIR_ERR_NO_DOMAIN, "%s", _("no domain with matching uuid")); + goto cleanup; + } + + if (virDomainObjIsActive(vm)) { + if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + } else { + if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) + flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; + /* check consistency between flags and the vm state */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("Domain is not running")); + goto cleanup; + } + } + + if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot modify device on transient domain")); + goto cleanup; + } + + priv = vm->privateData; + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + if (!(dev = virDomainDeviceDefParse(xml, vm->def, + driver->caps, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + /* Make a copy for updated domain. */ + if (!(vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, + driver->xmlopt))) + goto cleanup; + + if ((ret = libxlDomainUpdateDeviceConfig(vmdef, dev)) < 0) + goto cleanup; + } else { + ret = 0; + } + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + /* If dev exists it was created to modify the domain config. Free it. */ + virDomainDeviceDefFree(dev); + if (!(dev = virDomainDeviceDefParse(xml, vm->def, + driver->caps, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if ((ret = libxlDomainUpdateDeviceLive(priv, vm, dev)) < 0) + goto cleanup; + + /* + * update domain status forcibly because the domain status may be + * changed even if we attach the device failed. + */ + if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) + ret = -1; + } + + /* Finally, if no error until here, we can save config. */ + if (!ret && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) { + ret = virDomainSaveConfig(driver->configDir, vmdef); + if (!ret) { + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; + } + } + +cleanup: + virDomainDefFree(vmdef); + virDomainDeviceDefFree(dev); + if (vm) + virObjectUnlock(vm); + libxlDriverUnlock(driver); + return ret; } static unsigned long long diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index aafbbec..c43c7dd 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4131,17 +4131,9 @@ lxcDomainDetachDeviceLive(virLXCDriverPtr driver, } -/* Actions for lxcDomainModifyDeviceFlags */ -enum { - LXC_DEVICE_ATTACH, - LXC_DEVICE_UPDATE, - LXC_DEVICE_DETACH, -}; - - -static int -lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, - unsigned int flags, int action) +static int lxcDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) { virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; @@ -4151,9 +4143,7 @@ lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, unsigned int affect; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG | - (action == LXC_DEVICE_UPDATE ? - VIR_DOMAIN_DEVICE_MODIFY_FORCE : 0), -1); + VIR_DOMAIN_AFFECT_CONFIG, -1); affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); @@ -4215,23 +4205,7 @@ lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt); if (!vmdef) goto cleanup; - switch (action) { - case LXC_DEVICE_ATTACH: - ret = lxcDomainAttachDeviceConfig(vmdef, dev); - break; - case LXC_DEVICE_DETACH: - ret = lxcDomainDetachDeviceConfig(vmdef, dev); - break; - case LXC_DEVICE_UPDATE: - ret = lxcDomainUpdateDeviceConfig(vmdef, dev); - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown domain modify action %d"), action); - break; - } - - if (ret == -1) + if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0) goto cleanup; } @@ -4239,21 +4213,7 @@ lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) goto cleanup; - switch (action) { - case LXC_DEVICE_ATTACH: - ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy); - break; - case LXC_DEVICE_DETACH: - ret = lxcDomainDetachDeviceLive(driver, vm, dev_copy); - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown domain modify action %d"), action); - ret = -1; - break; - } - - if (ret == -1) + if ((ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy)) < 0) goto cleanup; /* * update domain status forcibly because the domain status may be @@ -4287,15 +4247,6 @@ cleanup: } -static int lxcDomainAttachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) -{ - return lxcDomainModifyDeviceFlags(dom, xml, flags, - LXC_DEVICE_ATTACH); -} - - static int lxcDomainAttachDevice(virDomainPtr dom, const char *xml) { @@ -4308,8 +4259,109 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - return lxcDomainModifyDeviceFlags(dom, xml, flags, - LXC_DEVICE_UPDATE); + virLXCDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainDefPtr vmdef = NULL; + virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + int ret = -1; + unsigned int affect; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG | + VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); + + affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); + + lxcDriverLock(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (virDomainObjIsActive(vm)) { + if (affect == VIR_DOMAIN_AFFECT_CURRENT) + flags |= VIR_DOMAIN_AFFECT_LIVE; + } else { + if (affect == VIR_DOMAIN_AFFECT_CURRENT) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + /* check consistency between flags and the vm state */ + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot do live update a device on " + "inactive domain")); + goto cleanup; + } + } + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot modify device on transient domain")); + goto cleanup; + } + + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, + driver->caps, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE); + if (dev == NULL) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG && + flags & VIR_DOMAIN_AFFECT_LIVE) { + /* If we are affecting both CONFIG and LIVE + * create a deep copy of device as adding + * to CONFIG takes one instance. + */ + dev_copy = virDomainDeviceDefCopy(dev, vm->def, + driver->caps, driver->xmlopt); + if (!dev_copy) + goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (virDomainDefCompatibleDevice(vm->def, dev) < 0) + goto cleanup; + + /* Make a copy for updated domain. */ + vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt); + if (!vmdef) + goto cleanup; + if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0) + goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + goto cleanup; + + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Unable to modify live devices")); + + goto cleanup; + } + + /* Finally, if no error until here, we can save config. */ + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + ret = virDomainSaveConfig(driver->configDir, vmdef); + if (!ret) { + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; + } + } + +cleanup: + virDomainDefFree(vmdef); + if (dev != dev_copy) + virDomainDeviceDefFree(dev_copy); + virDomainDeviceDefFree(dev); + if (vm) + virObjectUnlock(vm); + lxcDriverUnlock(driver); + return ret; } @@ -4317,8 +4369,116 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - return lxcDomainModifyDeviceFlags(dom, xml, flags, - LXC_DEVICE_DETACH); + virLXCDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainDefPtr vmdef = NULL; + virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + int ret = -1; + unsigned int affect; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); + + lxcDriverLock(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (virDomainObjIsActive(vm)) { + if (affect == VIR_DOMAIN_AFFECT_CURRENT) + flags |= VIR_DOMAIN_AFFECT_LIVE; + } else { + if (affect == VIR_DOMAIN_AFFECT_CURRENT) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + /* check consistency between flags and the vm state */ + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot do live update a device on " + "inactive domain")); + goto cleanup; + } + } + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot modify device on transient domain")); + goto cleanup; + } + + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, + driver->caps, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE); + if (dev == NULL) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG && + flags & VIR_DOMAIN_AFFECT_LIVE) { + /* If we are affecting both CONFIG and LIVE + * create a deep copy of device as adding + * to CONFIG takes one instance. + */ + dev_copy = virDomainDeviceDefCopy(dev, vm->def, + driver->caps, driver->xmlopt); + if (!dev_copy) + goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (virDomainDefCompatibleDevice(vm->def, dev) < 0) + goto cleanup; + + /* Make a copy for updated domain. */ + vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt); + if (!vmdef) + goto cleanup; + + if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev)) < 0) + goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + goto cleanup; + + if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev_copy)) < 0) + goto cleanup; + /* + * update domain status forcibly because the domain status may be + * changed even if we failed to attach the device. For example, + * a new controller may be created. + */ + if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) { + ret = -1; + goto cleanup; + } + } + + /* Finally, if no error until here, we can save config. */ + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + ret = virDomainSaveConfig(driver->configDir, vmdef); + if (!ret) { + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; + } + } + +cleanup: + virDomainDefFree(vmdef); + if (dev != dev_copy) + virDomainDeviceDefFree(dev_copy); + virDomainDeviceDefFree(dev); + if (vm) + virObjectUnlock(vm); + lxcDriverUnlock(driver); + return ret; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9abf6cb..6b4d3cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6287,23 +6287,14 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, return 0; } -/* Actions for qemuDomainModifyDeviceFlags */ -enum { - QEMU_DEVICE_ATTACH, - QEMU_DEVICE_DETACH, - QEMU_DEVICE_UPDATE, -}; - -static int -qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, - unsigned int flags, int action) +static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, + unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; - bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0; int ret = -1; unsigned int affect; virQEMUCapsPtr qemuCaps = NULL; @@ -6312,9 +6303,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, virCapsPtr caps = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG | - (action == QEMU_DEVICE_UPDATE ? - VIR_DOMAIN_DEVICE_MODIFY_FORCE : 0), -1); + VIR_DOMAIN_AFFECT_CONFIG, -1); cfg = virQEMUDriverGetConfig(driver); @@ -6382,23 +6371,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto endjob; - switch (action) { - case QEMU_DEVICE_ATTACH: - ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev); - break; - case QEMU_DEVICE_DETACH: - ret = qemuDomainDetachDeviceConfig(vmdef, dev); - break; - case QEMU_DEVICE_UPDATE: - ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, dev); - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown domain modify action %d"), action); - break; - } - - if (ret == -1) + if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0) goto endjob; } @@ -6406,24 +6379,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) goto endjob; - switch (action) { - case QEMU_DEVICE_ATTACH: - ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom); - break; - case QEMU_DEVICE_DETACH: - ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom); - break; - case QEMU_DEVICE_UPDATE: - ret = qemuDomainUpdateDeviceLive(dom->conn, vm, dev_copy, dom, force); - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown domain modify action %d"), action); - ret = -1; - break; - } - - if (ret == -1) + if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0) goto endjob; /* * update domain status forcibly because the domain status may be @@ -6462,12 +6418,6 @@ cleanup: return ret; } -static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, - unsigned int flags) -{ - return qemuDomainModifyDeviceFlags(dom, xml, flags, QEMU_DEVICE_ATTACH); -} - static int qemuDomainAttachDevice(virDomainPtr dom, const char *xml) { return qemuDomainAttachDeviceFlags(dom, xml, @@ -6479,13 +6429,265 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - return qemuDomainModifyDeviceFlags(dom, xml, flags, QEMU_DEVICE_UPDATE); + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainDefPtr vmdef = NULL; + virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0; + int ret = -1; + unsigned int affect; + virQEMUCapsPtr qemuCaps = NULL; + qemuDomainObjPrivatePtr priv; + virQEMUDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG | + VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); + + cfg = virQEMUDriverGetConfig(driver); + + affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + priv = vm->privateData; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjIsActive(vm)) { + if (affect == VIR_DOMAIN_AFFECT_CURRENT) + flags |= VIR_DOMAIN_AFFECT_LIVE; + } else { + if (affect == VIR_DOMAIN_AFFECT_CURRENT) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + /* check consistency between flags and the vm state */ + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot do live update a device on " + "inactive domain")); + goto endjob; + } + } + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot modify device on transient domain")); + goto endjob; + } + + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, + caps, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE); + if (dev == NULL) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG && + flags & VIR_DOMAIN_AFFECT_LIVE) { + /* If we are affecting both CONFIG and LIVE + * create a deep copy of device as adding + * to CONFIG takes one instance. + */ + dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); + if (!dev_copy) + goto endjob; + } + + if (priv->qemuCaps) + qemuCaps = virObjectRef(priv->qemuCaps); + else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator))) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (virDomainDefCompatibleDevice(vm->def, dev) < 0) + goto endjob; + + /* Make a copy for updated domain. */ + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + if (!vmdef) + goto endjob; + + if ((ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, dev)) < 0) + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + goto endjob; + + if ((ret = qemuDomainUpdateDeviceLive(dom->conn, vm, dev_copy, dom, force)) < 0) + goto endjob; + /* + * update domain status forcibly because the domain status may be + * changed even if we failed to attach the device. For example, + * a new controller may be created. + */ + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) { + ret = -1; + goto endjob; + } + } + + /* Finally, if no error until here, we can save config. */ + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + ret = virDomainSaveConfig(cfg->configDir, vmdef); + if (!ret) { + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; + } + } + +endjob: + if (qemuDomainObjEndJob(driver, vm) == 0) + vm = NULL; + +cleanup: + virObjectUnref(qemuCaps); + virDomainDefFree(vmdef); + if (dev != dev_copy) + virDomainDeviceDefFree(dev_copy); + virDomainDeviceDefFree(dev); + if (vm) + virObjectUnlock(vm); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; } + static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - return qemuDomainModifyDeviceFlags(dom, xml, flags, QEMU_DEVICE_DETACH); + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainDefPtr vmdef = NULL; + virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + int ret = -1; + unsigned int affect; + virQEMUCapsPtr qemuCaps = NULL; + qemuDomainObjPrivatePtr priv; + virQEMUDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + cfg = virQEMUDriverGetConfig(driver); + + affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + priv = vm->privateData; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjIsActive(vm)) { + if (affect == VIR_DOMAIN_AFFECT_CURRENT) + flags |= VIR_DOMAIN_AFFECT_LIVE; + } else { + if (affect == VIR_DOMAIN_AFFECT_CURRENT) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + /* check consistency between flags and the vm state */ + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot do live update a device on " + "inactive domain")); + goto endjob; + } + } + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot modify device on transient domain")); + goto endjob; + } + + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, + caps, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE); + if (dev == NULL) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG && + flags & VIR_DOMAIN_AFFECT_LIVE) { + /* If we are affecting both CONFIG and LIVE + * create a deep copy of device as adding + * to CONFIG takes one instance. + */ + dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); + if (!dev_copy) + goto endjob; + } + + if (priv->qemuCaps) + qemuCaps = virObjectRef(priv->qemuCaps); + else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator))) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (virDomainDefCompatibleDevice(vm->def, dev) < 0) + goto endjob; + + /* Make a copy for updated domain. */ + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + if (!vmdef) + goto endjob; + if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev)) < 0) + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + goto endjob; + + if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom)) < 0) + goto endjob; + /* + * update domain status forcibly because the domain status may be + * changed even if we failed to attach the device. For example, + * a new controller may be created. + */ + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) { + ret = -1; + goto endjob; + } + } + + /* Finally, if no error until here, we can save config. */ + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + ret = virDomainSaveConfig(cfg->configDir, vmdef); + if (!ret) { + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; + } + } + +endjob: + if (qemuDomainObjEndJob(driver, vm) == 0) + vm = NULL; + +cleanup: + virObjectUnref(qemuCaps); + virDomainDefFree(vmdef); + if (dev != dev_copy) + virDomainDeviceDefFree(dev_copy); + virDomainDeviceDefFree(dev); + if (vm) + virObjectUnlock(vm); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; } static int qemuDomainDetachDevice(virDomainPtr dom, const char *xml) -- 1.8.1.4
On 05/02/2013 06:03 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The LXC, QEMU, and LibXL drivers have all merged their handling of the attach/update/modify device APIs into one large
'xxxxDomainModifyDeviceFlags'
which then does a 'switch()' based on the actual API being invoked. While this saves some lines of code, it is not really all that significant in the context of the driver API impls as a whole.
This merger of the handling of different APIs creates pain when wanting to automated analysis of the code and do things which are specific to individual APIs. The slight duplication of code from unmerged the API impls, is preferrable to allow for easier automated analysis.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libxl/libxl_driver.c | 241 +++++++++++++++++++++++++++++------- src/lxc/lxc_driver.c | 278 ++++++++++++++++++++++++++++++++--------- src/qemu/qemu_driver.c | 316 ++++++++++++++++++++++++++++++++++++++--------- 3 files changed, 671 insertions(+), 164 deletions(-)
ACK. Looks huge, but it is mostly copy and paste of the existing code, followed by flattening out the switch back into the per-API call. And I agree that not multiplexing the code will make it easier to add ACL that allow device updates differently than device unplug. I still think you could reduce the overall code size by having helper functions for common tasks (such as flag sanitization of LIVE vs. CONFIG), instead of verbatim repetition of large preambles, but that can be separate patches. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The udevFreeIfaceDef function in the udev interface driver just duplicates code from virInterfaceDefFree. Delete it and call the standard API instead. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/interface/interface_backend_udev.c | 45 ++-------------------------------- 1 file changed, 2 insertions(+), 43 deletions(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 79c57ee..0384ef5 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -542,44 +542,6 @@ udevBridgeScanDirFilter(const struct dirent *entry) return 1; } -/** - * Frees any memory allocated by udevGetIfaceDef() - * - * @param ifacedef - interface to free and cleanup - */ -static void -udevFreeIfaceDef(virInterfaceDef *ifacedef) -{ - int i; - - if (!ifacedef) - return; - - if (ifacedef->type == VIR_INTERFACE_TYPE_BOND) { - VIR_FREE(ifacedef->data.bond.target); - for (i = 0; i < ifacedef->data.bond.nbItf; i++) { - udevFreeIfaceDef(ifacedef->data.bond.itf[i]); - } - VIR_FREE(ifacedef->data.bond.itf); - } - - if (ifacedef->type == VIR_INTERFACE_TYPE_BRIDGE) { - VIR_FREE(ifacedef->data.bridge.delay); - for (i = 0; i < ifacedef->data.bridge.nbItf; i++) { - udevFreeIfaceDef(ifacedef->data.bridge.itf[i]); - } - VIR_FREE(ifacedef->data.bridge.itf); - } - - if (ifacedef->type == VIR_INTERFACE_TYPE_VLAN) { - VIR_FREE(ifacedef->data.vlan.devname); - } - - VIR_FREE(ifacedef->mac); - VIR_FREE(ifacedef->name); - - VIR_FREE(ifacedef); -} static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) @@ -1080,7 +1042,7 @@ udevGetIfaceDef(struct udev *udev, const char *name) cleanup: udev_device_unref(dev); - udevFreeIfaceDef(ifacedef); + virInterfaceDefFree(ifacedef); return NULL; } @@ -1101,15 +1063,12 @@ udevInterfaceGetXMLDesc(virInterfacePtr ifinfo, */ ifacedef = udevGetIfaceDef(udev, ifinfo->name); - /* We've already printed by it happened */ if (!ifacedef) goto err; - /* Convert our interface to XML */ xmlstr = virInterfaceDefFormat(ifacedef); - /* Recursively free our interface structures and free the children too */ - udevFreeIfaceDef(ifacedef); + virInterfaceDefFree(ifacedef); err: /* decrement our udev ptr */ -- 1.8.1.4
On 05/02/2013 02:03 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The udevFreeIfaceDef function in the udev interface driver just duplicates code from virInterfaceDefFree. Delete it and call the standard API instead.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/interface/interface_backend_udev.c | 45 ++-------------------------------- 1 file changed, 2 insertions(+), 43 deletions(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 79c57ee..0384ef5 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -542,44 +542,6 @@ udevBridgeScanDirFilter(const struct dirent *entry) return 1; }
-/** - * Frees any memory allocated by udevGetIfaceDef() - * - * @param ifacedef - interface to free and cleanup - */ -static void -udevFreeIfaceDef(virInterfaceDef *ifacedef) -{ ... - - if (ifacedef->type == VIR_INTERFACE_TYPE_VLAN) { - VIR_FREE(ifacedef->data.vlan.devname); - }
virInterfaceDefFree also frees data.vlan.tag, which would result in freeing an invalid pointer, because it's created by a pretty ugly way in udevGetIfaceDefVlan: vlan_parent_dev = strdup(name); if (!vlan_parent_dev) { virReportOOMError(); goto cleanup; } /* Find the DEVICE.VID again */ vid = strrchr(vlan_parent_dev, '.'); if (!vid) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to find the VID for the VLAN device '%s'"), name); goto cleanup; } /* Replace the dot with a NULL so we can have the device and VID */ vid[0] = '\0'; vid++; /* Set the VLAN specifics */ ifacedef->data.vlan.tag = vid; ifacedef->data.vlan.devname = vlan_parent_dev; Jan
On Fri, May 03, 2013 at 10:44:24AM +0200, Ján Tomko wrote:
On 05/02/2013 02:03 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The udevFreeIfaceDef function in the udev interface driver just duplicates code from virInterfaceDefFree. Delete it and call the standard API instead.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/interface/interface_backend_udev.c | 45 ++-------------------------------- 1 file changed, 2 insertions(+), 43 deletions(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 79c57ee..0384ef5 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -542,44 +542,6 @@ udevBridgeScanDirFilter(const struct dirent *entry) return 1; }
-/** - * Frees any memory allocated by udevGetIfaceDef() - * - * @param ifacedef - interface to free and cleanup - */ -static void -udevFreeIfaceDef(virInterfaceDef *ifacedef) -{ ... - - if (ifacedef->type == VIR_INTERFACE_TYPE_VLAN) { - VIR_FREE(ifacedef->data.vlan.devname); - }
virInterfaceDefFree also frees data.vlan.tag, which would result in freeing an invalid pointer, because it's created by a pretty ugly way in udevGetIfaceDefVlan:
vlan_parent_dev = strdup(name); if (!vlan_parent_dev) { virReportOOMError(); goto cleanup; }
/* Find the DEVICE.VID again */ vid = strrchr(vlan_parent_dev, '.'); if (!vid) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to find the VID for the VLAN device '%s'"), name); goto cleanup; }
/* Replace the dot with a NULL so we can have the device and VID */ vid[0] = '\0'; vid++;
/* Set the VLAN specifics */ ifacedef->data.vlan.tag = vid; ifacedef->data.vlan.devname = vlan_parent_dev;
My god that code must die a horrible death. It is currently leaking memory by doing this. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko