On 11/22/2012 05:48 PM, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

The docs for virDiskNameToIndex claim it ignores partition numbers. In actual fact though, a code ordering buy means

s/buy/bug/

that a partition number will cause the code to accidentally multiply the result by 26.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/util.c b/src/util/util.c index 75b18c1..2fd0f2c 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2193,11 +2193,10 @@ int virDiskNameToIndex(const char *name) { return -1;

for (i = 0; *ptr; i++) { - idx = (idx + (i < 1 ? 0 : 1)) * 26; - if (!c_islower(*ptr)) break;

+ idx = (idx + (i < 1 ? 0 : 1)) * 26; idx += *ptr - 'a'; ptr++; }

ACK, Martin

s/non-existant/non-existent/ in the subject On 11/22/12 17:48, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

The event code is a no-op if requested to update a non-existant

s/non-existant/non-existent/

timer/handle watch. This makes it hard to detect bugs in the caller who have passed bogus data. Add a VIR_WARN output in such cases, since the API does not allow for return errors.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/event_poll.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)

diff --git a/src/util/event_poll.c b/src/util/event_poll.c index 276d66d..2ffa94b 100644 --- a/src/util/event_poll.c +++ b/src/util/event_poll.c @@ -141,6 +141,7 @@ int virEventPollAddHandle(int fd, int events,

void virEventPollUpdateHandle(int watch, int events) { int i; + bool found = false; PROBE(EVENT_POLL_UPDATE_HANDLE, "watch=%d events=%d", watch, events); @@ -156,10 +157,14 @@ void virEventPollUpdateHandle(int watch, int events) { eventLoop.handles[i].events = virEventPollToNativeEvents(events); virEventPollInterruptLocked(); + found = true; break; } } virMutexUnlock(&eventLoop.lock); + + if (!found) + VIR_WARN("Got update for non-existant handle watch %d", watch);

s/non-existant/non-existent/

}

/* @@ -249,6 +254,7 @@ void virEventPollUpdateTimeout(int timer, int frequency) { unsigned long long now; int i; + bool found = false; PROBE(EVENT_POLL_UPDATE_TIMEOUT, "timer=%d frequency=%d", timer, frequency); @@ -268,11 +274,17 @@ void virEventPollUpdateTimeout(int timer, int frequency) eventLoop.timeouts[i].frequency = frequency; eventLoop.timeouts[i].expiresAt = frequency >= 0 ? frequency + now : 0; + VIR_DEBUG("Set timer freq=%d expirs=%llu", frequency,

s/expirs/expires/

+ eventLoop.timeouts[i].expiresAt); virEventPollInterruptLocked(); + found = true; break; } } virMutexUnlock(&eventLoop.lock); + + if (!found) + VIR_WARN("Got update for non-existant timer %d", timer);

s/non-existant/non-existent/

}

/* @@ -336,6 +348,7 @@ static int virEventPollCalculateTimeout(int *timeout) { if (virTimeMillisNow(&now) < 0) return -1;

+ EVENT_DEBUG("Schedule timeout then=%llu now=%llu", then, now); *timeout = then - now; if (*timeout < 0) *timeout = 0;

ACK with spelling and the typo fixed. Peter

On 11/22/2012 05:48 PM, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

It is possible for there to be deleted timers when we calculate the next timeout, and they must be skipped.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/event_poll.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/src/util/event_poll.c b/src/util/event_poll.c index 2ffa94b..53b9c6c 100644 --- a/src/util/event_poll.c +++ b/src/util/event_poll.c @@ -332,6 +332,8 @@ static int virEventPollCalculateTimeout(int *timeout) { EVENT_DEBUG("Calculate expiry of %zu timers", eventLoop.timeoutsCount); /* Figure out if we need a timeout */ for (i = 0 ; i < eventLoop.timeoutsCount ; i++) { + if (eventLoop.timeouts[i].deleted) + continue; if (eventLoop.timeouts[i].frequency < 0) continue;

ACK, Martin

On 11/22/2012 05:48 PM, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

The virLXCControllerClientCloseHook method was mistakenly assuming that the private data associated with the network client was the virLXCControllerPtr. In fact it was just a dummy int, so we were derefencing a bogus struct. The frequent result of this was that we would never quit, because we tried to arm a non-existant timer.

Fix the code by removing the dummy private data and just using the virLXCControllerPtr instance as private data

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 6fffd68..a9d2d40 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -578,19 +578,14 @@ static void virLXCControllerClientCloseHook(virNetServerClientPtr client)

static void virLXCControllerClientPrivateFree(void *data) { - VIR_FREE(data); + virLXCControllerPtr ctrl = data; + VIR_DEBUG("Got private data free %p", ctrl); }

static void *virLXCControllerClientPrivateNew(virNetServerClientPtr client, void *opaque) { virLXCControllerPtr ctrl = opaque; - int *dummy; - - if (VIR_ALLOC(dummy) < 0) { - virReportOOMError(); - return NULL; - }

virNetServerClientSetCloseHook(client, virLXCControllerClientCloseHook); VIR_DEBUG("Got new client %p", client); @@ -600,7 +595,7 @@ static void *virLXCControllerClientPrivateNew(virNetServerClientPtr client, virLXCControllerEventSendInit(ctrl, ctrl->initpid); ctrl->firstClient = false;

- return dummy; + return ctrl; }

@@ -1327,7 +1322,7 @@ virLXCControllerEventSendExit(virLXCControllerPtr ctrl, { virLXCProtocolExitEventMsg msg;

- VIR_DEBUG("Exit status %d", exitstatus); + VIR_DEBUG("Exit status %d (client=%p)", exitstatus, ctrl->client); memset(&msg, 0, sizeof(msg)); switch (exitstatus) { case 0:

ACK, Martin

On 11/22/2012 11:48 AM, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

The impls of virSecurityManagerGetMountOptions had no way to return errors, since the code was treating 'NULL' as a success value. This is somewhat pointless, since the calling code did not want NULL in the first place and has to translate it into the empty string "". So change the code so that the impls can return "" directly, allowing use of NULL for error reporting once again

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_container.c | 10 ++++++---- src/security/security_apparmor.c | 17 +++++++++++++++++ src/security/security_manager.c | 5 +---- src/security/security_nop.c | 15 +++++++++++++-- src/security/security_selinux.c | 28 +++++++++++++++++----------- 5 files changed, 54 insertions(+), 21 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index db1f6ed..ebeaca1 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -571,7 +571,7 @@ static int lxcContainerMountBasicFS(bool pivotRoot, */

ignore_value(virAsprintf(&opts, - "mode=755,size=65536%s",(sec_mount_options ? sec_mount_options : ""))); + "mode=755,size=65536%s", sec_mount_options)); if (!opts) { virReportOOMError(); goto cleanup; @@ -1083,7 +1083,7 @@ static int lxcContainerMountFSTmpfs(virDomainFSDefPtr fs, char *data = NULL;

if (virAsprintf(&data, - "size=%lldk%s", fs->usage, (sec_mount_options ? sec_mount_options : "")) < 0) { + "size=%lldk%s", fs->usage, sec_mount_options) < 0) { virReportOOMError(); goto cleanup; } @@ -1456,7 +1456,7 @@ static int lxcContainerMountCGroups(struct lxcContainerCGroup *mounts, }

if (virAsprintf(&opts, - "mode=755,size=65536%s",(sec_mount_options ? sec_mount_options : "")) < 0) { + "mode=755,size=65536%s", sec_mount_options) < 0) { virReportOOMError(); return -1; } @@ -1689,7 +1689,9 @@ static int lxcContainerSetupMounts(virDomainDefPtr vmDef, if (lxcContainerResolveSymlinks(vmDef) < 0) return -1;

- sec_mount_options = virSecurityManagerGetMountOptions(securityDriver, vmDef); + if (!(sec_mount_options = virSecurityManagerGetMountOptions(securityDriver, vmDef))) + return -1; + if (root && root->src) rc = lxcContainerSetupPivotRoot(vmDef, root, ttyPaths, nttyPaths, sec_mount_options); else diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 1315fe1..b0cdb65 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -881,6 +881,21 @@ AppArmorSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; }

+ +static char * +AppArmorGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED) +{ + char *opts; + + if (!(opts = strdup(""))) { + virReportOOMError(); + return NULL; + } + return opts; +} + + virSecurityDriver virAppArmorSecurityDriver = { .privateDataLen = 0, .name = SECURITY_APPARMOR_NAME, @@ -918,4 +933,6 @@ virSecurityDriver virAppArmorSecurityDriver = {

.domainSetSecurityImageFDLabel = AppArmorSetImageFDLabel, .domainSetSecurityTapFDLabel = AppArmorSetTapFDLabel, + + .domainGetSecurityMountOptions = AppArmorGetMountOptions, }; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index d446607..0ebd53b 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -486,10 +486,7 @@ char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, if (mgr->drv->domainGetSecurityMountOptions) return mgr->drv->domainGetSecurityMountOptions(mgr, vm);

- /* - I don't think this is an error, these should be optional - virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); - */ + virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return NULL; }

diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 86f644b..5f3270a 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -21,6 +21,10 @@

#include "security_nop.h"

+#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_SECURITY + static virSecurityDriverStatus virSecurityDriverProbeNop(const char *virtDriver ATTRIBUTE_UNUSED) { return SECURITY_DRIVER_ENABLE; @@ -165,8 +169,15 @@ static int virSecurityDomainSetFDLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UN }

static char *virSecurityDomainGetMountOptionsNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED) { - return NULL; + virDomainDefPtr vm ATTRIBUTE_UNUSED) +{ + char *opts; + + if (!(opts = strdup(""))) { + virReportOOMError(); + return NULL; + } + return opts; }

virSecurityDriver virSecurityDriverNop = { diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 8fcaaa8..0e49ded 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1974,20 +1974,26 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr, char *opts = NULL; virSecurityLabelDefPtr secdef;

- secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return NULL; - - if (! secdef->imagelabel) - secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr,def); + if ((secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME))) { + if (!secdef->imagelabel) + secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr,def);

Missing space after comma (was already there, but might as well fix it while you're moving stuff).

+ + if (secdef->imagelabel && + virAsprintf(&opts, + ",context=\"%s\"", + (const char*) secdef->imagelabel) < 0) { + virReportOOMError(); + return NULL; + } + }

- if (secdef->imagelabel) { - virAsprintf(&opts, - ",context=\"%s\"", - (const char*) secdef->imagelabel); + if (!opts && + !(opts = strdup(""))) { + virReportOOMError(); + return NULL; }

- VIR_DEBUG("imageLabel=%s", secdef->imagelabel); + VIR_DEBUG("imageLabel=%s opts=%s", secdef->imagelabel, opts); return opts; }

ACK with the space added.

On 11/22/2012 11:48 AM, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

Currently the lxcContainerSetupMounts method uses the virSecurityManagerPtr instance to obtain the mount options string and then only passes the string down into methods it calls. As functionality in LXC grows though, those methods need to have direct access to the virSecurityManagerPtr instance. So push the code down a level.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_container.c | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index ebeaca1..8e2e3ec 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1511,17 +1511,21 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, virDomainFSDefPtr root, char **ttyPaths, size_t nttyPaths, - char *sec_mount_options) + virSecurityManagerPtr securityDriver) { struct lxcContainerCGroup *mounts = NULL; size_t nmounts = 0; int ret = -1; - char *cgroupRoot; + char *cgroupRoot = NULL; + char *sec_mount_options; + + if (!(sec_mount_options = virSecurityManagerGetMountOptions(securityDriver, vmDef))) + return -1;

/* Before pivoting we need to identify any * cgroups controllers that are mounted */ if (lxcContainerIdentifyCGroups(&mounts, &nmounts, &cgroupRoot) < 0) - return -1; + goto cleanup;

/* Gives us a private root, leaving all parent OS mounts on /.oldroot */ if (lxcContainerPivotRoot(root) < 0) @@ -1577,6 +1581,7 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, cleanup: lxcContainerCGroupFree(mounts, nmounts); VIR_FREE(cgroupRoot); + VIR_FREE(sec_mount_options); return ret; }

@@ -1585,14 +1590,19 @@ cleanup: but with extra stuff mapped in */ static int lxcContainerSetupExtraMounts(virDomainDefPtr vmDef, virDomainFSDefPtr root, - char *sec_mount_options) + virSecurityManagerPtr securityDriver) { int ret = -1; struct lxcContainerCGroup *mounts = NULL; size_t nmounts = 0; - char *cgroupRoot; + char *cgroupRoot = NULL; + char *sec_mount_options;

VIR_DEBUG("def=%p", vmDef); + + if (!(sec_mount_options = virSecurityManagerGetMountOptions(securityDriver, vmDef))) + return -1; + /* * This makes sure that any new filesystems in the * host OS propagate to the container, but any @@ -1601,25 +1611,25 @@ static int lxcContainerSetupExtraMounts(virDomainDefPtr vmDef, if (mount("", "/", NULL, MS_SLAVE|MS_REC, NULL) < 0) { virReportSystemError(errno, "%s", _("Failed to make / slave")); - return -1; + goto cleanup; }

if (root && root->readonly) { if (mount("", "/", NULL, MS_BIND|MS_REC|MS_RDONLY|MS_REMOUNT, NULL) < 0) { virReportSystemError(errno, "%s", _("Failed to make root readonly")); - return -1; + goto cleanup; } }

VIR_DEBUG("Mounting config FS"); if (lxcContainerMountAllFS(vmDef, "", false, sec_mount_options) < 0) - return -1; + goto cleanup;

/* Before replacing /sys we need to identify any * cgroups controllers that are mounted */ if (lxcContainerIdentifyCGroups(&mounts, &nmounts, &cgroupRoot) < 0) - return -1; + goto cleanup;

#if HAVE_SELINUX /* Some versions of Linux kernel don't let you overmount @@ -1653,6 +1663,7 @@ static int lxcContainerSetupExtraMounts(virDomainDefPtr vmDef, cleanup: lxcContainerCGroupFree(mounts, nmounts); VIR_FREE(cgroupRoot); + VIR_FREE(sec_mount_options); return ret; }

@@ -1684,21 +1695,15 @@ static int lxcContainerSetupMounts(virDomainDefPtr vmDef, size_t nttyPaths, virSecurityManagerPtr securityDriver) { - int rc = -1; - char *sec_mount_options = NULL; if (lxcContainerResolveSymlinks(vmDef) < 0) return -1;

- if (!(sec_mount_options = virSecurityManagerGetMountOptions(securityDriver, vmDef))) - return -1; - if (root && root->src) - rc = lxcContainerSetupPivotRoot(vmDef, root, ttyPaths, nttyPaths, sec_mount_options); + return lxcContainerSetupPivotRoot(vmDef, root, ttyPaths, nttyPaths, + securityDriver); else - rc = lxcContainerSetupExtraMounts(vmDef, root, sec_mount_options); - - VIR_FREE(sec_mount_options); - return rc; + return lxcContainerSetupExtraMounts(vmDef, root, + securityDriver); }

ACK.