September 2009 - Devel - libvirt List Archives

[libvirt] [PATCH] openvz_driver.c: avoid dead store to "err"
by Jim Meyering 07 Sep '09

07 Sep '09

>From 559c1ba31a89fd3a6ed54631d5d900fa16140187 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 19:11:31 +0200 Subject: [PATCH] openvz_driver.c: avoid dead store to "err" * src/openvz_driver.c (openvzGenerateContainerVethName): Remove use and decl of "err". --- src/openvz_driver.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/openvz_driver.c b/src/openvz_driver.c index 43d54f9..a8c24ba 100644 --- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -504,11 +504,10 @@ openvzGenerateVethName(int veid, char *dev_name_ve) static char * openvzGenerateContainerVethName(int veid) { - int ret; char temp[1024]; /* try to get line "^NETIF=..." from config */ - if ( (ret = openvzReadVPSConfigParam(veid, "NETIF", temp, sizeof(temp))) <= 0) { + if (openvzReadVPSConfigParam(veid, "NETIF", temp, sizeof(temp)) <= 0) { snprintf(temp, sizeof(temp), "eth0"); } else { char *saveptr; -- 1.6.4.2.409.g85dc3

2 1

[libvirt] 3 more dead stores
by Jim Meyering 07 Sep '09

07 Sep '09

>From 7a2202539b6445017c420644e5327ce64eaf3bac Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 17:27:34 +0200 Subject: [PATCH 1/2] domain_conf.c: remove two dead stores * src/domain_conf.c (virDomainSaveXML): Remove use and decl of "err". (virDomainDefParseXML): Likewise. --- src/domain_conf.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 8dde5dd..050cf50 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -2520,8 +2520,7 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, /* Extract domain uuid */ tmp = virXPathString(conn, "string(./uuid[1])", ctxt); if (!tmp) { - int err; - if ((err = virUUIDGenerate(def->uuid))) { + if (virUUIDGenerate(def->uuid)) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); goto error; @@ -4456,12 +4455,11 @@ int virDomainSaveXML(virConnectPtr conn, char *configFile = NULL; int fd = -1, ret = -1; size_t towrite; - int err; if ((configFile = virDomainConfigFile(conn, configDir, def->name)) == NULL) goto cleanup; - if ((err = virFileMakePath(configDir))) { + if (virFileMakePath(configDir)) { virReportSystemError(conn, errno, _("cannot create config directory '%s'"), configDir); -- 1.6.4.2.409.g85dc3 >From 89e2837b4382690e4d434c086541da1dcd3d4c58 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 18:53:20 +0200 Subject: [PATCH 2/2] util.c: avoid dead store to "flag" * src/util.c (virExecDaemonize): Change flag |= VAR to "flag | VAR". --- src/util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util.c b/src/util.c index 2529837..af50028 100644 --- a/src/util.c +++ b/src/util.c @@ -663,7 +663,7 @@ int virExecDaemonize(virConnectPtr conn, ret = virExecWithHook(conn, argv, envp, keepfd, retpid, infd, outfd, errfd, - flags |= VIR_EXEC_DAEMON, + flags | VIR_EXEC_DAEMON, hook, data, pidfile); /* __virExec should have set an error */ -- 1.6.4.2.409.g85dc3

2 1

Re: [libvirt] [PATCH 0/6] sVirt AppArmor security driver
by Daniel Veillard 07 Sep '09

07 Sep '09

[ Sending again as my mail Sat, Sep 05, 2009 at 11:00:46AM +0200 didn't seem to have reach the list, Daniel ] On Fri, Sep 04, 2009 at 04:03:50PM -0500, Jamie Strandboge wrote: > On Fri, 04 Sep 2009, Daniel Veillard wrote: > > > On Fri, Sep 04, 2009 at 12:23:33PM -0500, Jamie Strandboge wrote: > > > This patch series implements the AppArmor security driver for sVirt. > > > This implementation was developed for the Ubuntu AppArmorLibvirtProfile > > > specification[1], but is general enough for any AppArmor deployment > > > (such as Ubuntu, *SUSE and Mandriva). > > > > > > This patch has seen quite a bit of real world testing in Ubuntu 9.10 > > > (our development release) in our 0.7.0-1ubuntu3 package. I did make a > > > few small changes after going through HACKING, but mostly I got the > > > tests going and added documentation. > > > > Could you triple check that the make syntax-check run clean with > > your patches applied, because a very quick glimpse shows up a number > > of malloc() which we don't allow for example. It may be because you > > didn't enter the files in the SCM but the checks should be done anyway > > on the new files please. > > > > I noticed that syntax-check wouldn't scan the new files unless I did a > git commit first. I did not run 'syntax-check' after adding the > documention examples, and just found that examples/apparmor/libvirt-qemu > had a trailing blank line (I was a bit surprised syntax-check checked > these files (attached is an updated docs patch)). okay > I can demonstrate that the files are being checked by removing the > '#include <config.h>' line in security_apparmor.c and virt-aa-helper.c > and syntax-check failing: [...] > I don't see that 'syntax-check' enforces not using *alloc, but I did > read in HACKING that *alloc are deprecated, though I had already written > the code before reading it. I do see other examples of *alloc in the I really though all cases of direct malloc or realloc had been eliminated, apparently it's not the case yet, and syntax-check doesn't enforce it... we need to do this. I note we also still have a few free() left around in the code base to cleanup too. > codebase, and I do properly check the return code of all calls to *alloc. > If you insist on those calls being replaced, I can do that, but I didn't > before submitting because I knew the current calls were correct and > didn't want to introduce bugs into the code. yeah, I think it's better to fix them before. In any case we have a week of bug fix only before the next release in a week, so best is to fix them now, wait for the actual reviews and then resubmit patches for inclusion. thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

1 0

[libvirt] Some close/fclose/closedir calls are missing
by Matthias Bolte 07 Sep '09

07 Sep '09

While debugging something that looked like an file descriptor leak, we (Maximilian Wilhelm and me) found some places where close/fclose/closedir calls are missing, mostly in error handling if-branches. See attached patch. Max and Matthias

3 2

[libvirt] [PATCH 0/6] sVirt AppArmor security driver
by Jamie Strandboge 07 Sep '09

07 Sep '09

This patch series implements the AppArmor security driver for sVirt. This implementation was developed for the Ubuntu AppArmorLibvirtProfile specification[1], but is general enough for any AppArmor deployment (such as Ubuntu, *SUSE and Mandriva). This patch has seen quite a bit of real world testing in Ubuntu 9.10 (our development release) in our 0.7.0-1ubuntu3 package. I did make a few small changes after going through HACKING, but mostly I got the tests going and added documentation. DESIGN ------ When a virtual machine is started, determine if a profile is currently defined for the machine, and use it if available. If not, generate a new profile for the machine based on a template, which is by default a very restrictive profile allowing access to disk files, and anything else needed to run, such as the pid, monitor and log files. Virtual machines should have a unique profile specific to that machine. To ensure uniqueness, the profile name will be derived from the UUID of the virtual machine. These profiles should be configurable, either by adjusting the profile template for new machines, creating/modifying the VM profile directly or through the use of AppArmor abstractions. This will allow for administrators to fine-tune confinement for individual machines if desired. If enabled at compile time, the sVirt security model will be activated if AppArmor is available on the host OS and a profile for the libvirtd daemon is loaded when libvirtd is started. libvirtd should not be allowed to create arbitrary profiles or modify profiles directly, so as to not allow libvirtd to potentially (ie via a security bug in libvirtd itself) bootstrap out of AppArmor confinement. Because root privileges are needed to manipulate AppArmor profiles, qemu:///session will not be supported at this time, but the implementation must allow for a confined libvirtd with qemu:///session guests running unconfined. This can be revisited when AppArmor supports per-user profiles. Please see the specification[1] for more details. PATCHES ------- The patches are all against trunk as of yesterday. Testing was done on trunk and there seem to be no regressions over the the 0.7.0-1ubuntu3 package in Ubuntu. [PATCH 1*] patch_1a_reenable-nonfile-labels.patch: When James Morris originally submitted his sVirt patches (as seen in libvirt 0.6.1), he did not require on disk labelling for virSecurityDomainRestoreImageLabel. A later commit[2] changed this behavior to assume on disk labelling, which halts implementations for path-based MAC systems such as AppArmor and TOMOYO where vm->def->seclabel is required to obtain the label. This patch simply adds the 'virDomainObjPtr vm' argument back to *RestoreImageLabel. patch_1b_optional.patch: Due to the above change, 'make syntax-check' fails because SELinuxRestoreSecurityImageLabel() does not use the 'virDomainObjPtr vm'. patch_1b_optional.patch is a simple patch to fix this by checking if vm->def->seclabel == NULL and returns with error if it does. I realize this may not be desired in the long term, but it should be harmless enough to include. [PATCH 2] patch_2_security_c.patch: Updates src/security.c for AppArmor [PATCH 3] patch_3_security_apparmor.patch: Adds security_apparmor.c, security_apparmor.h, virt-aa-helper.c and updates po/POTFILES.in. virt-aa-helper.c is a new binary which is used exclusively by the AppArmor security driver to manipulate AppArmor. These files compile without warning and pass syntax-check. [PATCH 4] patch_4_tests.patch: Adds tests for virt-aa-helper and the security driver. secaatest.c is identical to seclabeltest.c except it initializes the 'apparmor' driver instead of 'selinux'. These tests are integrated into 'make check' and pass. [PATCH 5] patch_5_docs.patch: Updates docs/drvqemu.html.in for AppArmor and adds profile examples to examples/apparmor. [PATCH 6] patch_6_autoconf.patch: Updates Makefile.am and configure.in for AppArmor. It is based on and should operate the same as the SELinux configuration. Caveats and known issues: 1. it does not take advantage of the recent host device labelling functionality yet 2. it does not properly handle hot-plugging of devices yet 3. qemu:///session runs unconfined (see above) Thanks! Jamie (jdstrand on Freenode and OFTC) [1] https://wiki.ubuntu.com/SecurityTeam/Specifications/AppArmorLibvirtProfile [2] http://libvirt.org/git/?p=libvirt.git;a=commit;h=c86afc85ee0d1ec6d76c2d254b… -- Jamie Strandboge | http://www.canonical.com

3 10

[libvirt] [PATCH 0/6] Automatic sVirt management of host device labelling
by Daniel P. Berrange 07 Sep '09

07 Sep '09

This patch series extends the libvirt security driver API, and sVirt implementation to cover management of host device labelling. Previously users would have to set a global boolean tunable virt_use_pci/usb to allow all domains access to all host devices. With this series applied libvirt will automatically relabel only the individual PCI/USB devices which are assigned to a guest. ie it should make host device assignment 'just work' when sVirt is enforcing, and improve security It also attempts to address a problem with restoration of disk labels. The current code uses matchpathcon() to find the defalt label for a path. This works fine for locations which have a defined label in the policy (eg like /var/lib/libvirt/images), but if storing disk images in non-defualt locations, eg a external USB drive mounted under a place like /media/myusbdisk/virtual-images/, matchpathcon() returns NULL. In this scenario the disk would remain labelled with the MCS level specific to the just stopped VM. Since MCS labels are allocated on demand on each boot, this could allow a future VMs to access disks that it ought not to be able to. Dan Walsh suggested that we default to using the label defined for matchpathcon("/var/libvirt/images/00-DEFAULT") in this case, but this doesn't work for restoring USB/PCI device labels[1]. In all the case I've had this problem so far, the files' original label matched that of the directory it was contained in, so this patch just uses the containing directory's label when restoring labels. Dan didn't like this idea when I first mentioned it in IRC though, so perhaps I need to implement different logic still... ? Regards, Daniel [1] PCI device access from VMs requires labelling /sys/bus/pci/devices/$DOMAIN:$BUS:$SLOT:FUNCTION/{config, resource*, rom} while USB device access requires labelling /dev/bus/usb/$BUS/$DEVICE

5 18

[libvirt] [PATCH] Fix several memory leaks
by Ryota Ozaki 05 Sep '09

05 Sep '09

Hi, This patch fixes several memory leaks being scattered over libvirt. Thanks, ozaki-r >From 6fc282eae5192cccda208d8d4fd14c5c0676992b Mon Sep 17 00:00:00 2001 From: Ryota Ozaki <ozaki.ryota(a)gmail.com> Date: Tue, 1 Sep 2009 01:34:40 +0900 Subject: [PATCH] Fix several memory leaks --- qemud/qemud.c | 2 +- src/domain_conf.c | 1 + src/network_conf.c | 5 +++-- src/qemu_conf.c | 13 ++++++++----- src/storage_backend_fs.c | 5 +++++ 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/qemud/qemud.c b/qemud/qemud.c index df275e6..17ba44a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1733,7 +1733,7 @@ readmore: /* Possibly need to create another receive buffer */ if ((client->nrequests < max_client_requests && - VIR_ALLOC(client->rx) < 0)) { + !client->rx && VIR_ALLOC(client->rx) < 0)) { qemudDispatchClientFailure(client); } else { if (client->rx) diff --git a/src/domain_conf.c b/src/domain_conf.c index 1d2cc7c..4b64219 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -4361,6 +4361,7 @@ int virDomainSaveXML(virConnectPtr conn, cleanup: if (fd != -1) close(fd); + VIR_FREE(configFile); return ret; } diff --git a/src/network_conf.c b/src/network_conf.c index bb649a4..58a4f32 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn, { char *configFile = NULL; char *autostartLink = NULL; + int ret = -1; if ((configFile = virNetworkConfigFile(conn, configDir, net->def->name)) == NULL) goto error; @@ -836,12 +837,12 @@ int virNetworkDeleteConfig(virConnectPtr conn, goto error; } - return 0; + ret = 0; error: VIR_FREE(configFile); VIR_FREE(autostartLink); - return -1; + return ret; } char *virNetworkConfigFile(virConnectPtr conn, diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 22f5edd..32d6a48 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1034,7 +1034,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, virDomainNetDefPtr net, int qemuCmdFlags) { - char *brname; + char *brname = NULL; int err; int tapfd = -1; int vnet_hdr = 0; @@ -1053,7 +1053,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (brname == NULL) return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - brname = net->data.bridge.brname; + brname = strdup(net->data.bridge.brname); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Network type %d is not supported"), net->type); @@ -1063,7 +1063,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (!driver->brctl && (err = brInit(&driver->brctl))) { virReportSystemError(conn, err, "%s", _("cannot initialize bridge support")); - return -1; + goto cleanup; } if (!net->ifname || @@ -1072,7 +1072,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, VIR_FREE(net->ifname); if (!(net->ifname = strdup("vnet%d"))) { virReportOOMError(conn); - return -1; + goto cleanup; } /* avoid exposing vnet%d in dumpxml or error outputs */ template_ifname = 1; @@ -1100,9 +1100,12 @@ qemudNetworkIfaceConnect(virConnectPtr conn, } if (template_ifname) VIR_FREE(net->ifname); - return -1; + tapfd = -1; } +cleanup: + VIR_FREE(brname); + return tapfd; } diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ca6d329..222e591 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, vol->type = VIR_STORAGE_VOL_FILE; vol->target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value is filled in during probe */ + if (vol->target.path) + VIR_FREE(vol->target.path); if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) @@ -1022,6 +1024,9 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, vol->type = VIR_STORAGE_VOL_FILE; + if (vol->target.path) + VIR_FREE(vol->target.path); + if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) { -- 1.6.0.6

4 7

[libvirt] [PATCH] storage_driver.c: remove two useless calls to virStorageBackendForType
by Jim Meyering 04 Sep '09

04 Sep '09

>From dead-store warnings: >From c925b02828083a016f7b9a7ae93eb3a7bf5f4609 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 17:18:29 +0200 Subject: [PATCH] storage_driver.c: remove two useless calls to virStorageBackendForType * src/storage_driver.c (storagePoolGetInfo, storagePoolDefine): The result was never used, and the function has no side effects. --- src/storage_driver.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-) diff --git a/src/storage_driver.c b/src/storage_driver.c index e9ecb20..e8d43b7 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -519,15 +519,11 @@ storagePoolDefine(virConnectPtr conn, virStoragePoolDefPtr def; virStoragePoolObjPtr pool = NULL; virStoragePoolPtr ret = NULL; - virStorageBackendPtr backend; storageDriverLock(driver); if (!(def = virStoragePoolDefParseString(conn, xml))) goto cleanup; - if ((backend = virStorageBackendForType(def->type)) == NULL) - goto cleanup; - if (!(pool = virStoragePoolObjAssignDef(conn, &driver->pools, def))) goto cleanup; @@ -847,7 +843,6 @@ storagePoolGetInfo(virStoragePoolPtr obj, virStoragePoolInfoPtr info) { virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; - virStorageBackendPtr backend; int ret = -1; storageDriverLock(driver); @@ -860,9 +855,6 @@ storagePoolGetInfo(virStoragePoolPtr obj, goto cleanup; } - if ((backend = virStorageBackendForType(pool->def->type)) == NULL) - goto cleanup; - memset(info, 0, sizeof(virStoragePoolInfo)); if (pool->active) info->state = VIR_STORAGE_POOL_RUNNING; -- 1.6.4.2.409.g85dc3

2 3

[libvirt] [PATCH 6/7] VMware ESX: Add esxDomainUndefine() based on esxVI_UnregisterVM()
by Matthias Bolte 04 Sep '09

04 Sep '09

* src/esx/esx_driver.c: add esxDomainUndefine() based on esxVI_UnregisterVM() * src/esx/esx_vi_methods.[ch]: add esxVI_UnregisterVM()

2 3

[libvirt] [PATCH 5/7] VMware ESX: Add esxGetCapabilities() with basic default capabilities
by Matthias Bolte 04 Sep '09

04 Sep '09

* src/esx/esx_driver.c: add esxCapsInit() with default caps and add esxGetCapabilities()

2 3