Re: [libvirt] [PATCH] vbox: fix VIR_STRDUP value check
On 05/20/2013 05:08 PM, Michal Privoznik wrote:
On 20.05.2013 11:59, Ján Tomko wrote:
In my review of 31532ca I missed the fact that VIR_STRDUP
now returns 1 on success, and 0 if the source was NULL.
(This still doesn't add proper OOM error handling.)
---
src/vbox/vbox_tmpl.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index eb8ac63..163aeff 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -2688,9 +2688,9 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom,
unsigned int flags) {
if (hddType == HardDiskType_Immutable)
def-disks[hddNum]-readonly = true;
-if (VIR_STRDUP_QUIET(def-disks[hddNum]-src, hddlocation)
== 0
-VIR_STRDUP_QUIET(def-disks[hddNum]-dst, hdd) == 0)
-hddNum++;
+ignore_value(VIR_STRDUP(def-disks[hddNum]-src,
hddlocation));
+ignore_value(VIR_STRDUP(def-disks[hddNum]-dst, hdd));
+hddNum++;
VBOX_UTF8_FREE(hddlocation);
VBOX_UTF16_FREE(hddlocationUtf16);
@@ -7507,7 +7507,7 @@ static int vboxConnectListNetworks(virConnectPtr conn,
char **const names, int n
VBOX_UTF16_TO_UTF8(nameUtf16, nameUtf8);
VIR_DEBUG(nnames[%d]: %s, ret, nameUtf8);
-if (VIR_STRDUP(names[ret], nameUtf8) == 0)
+if (VIR_STRDUP(names[ret], nameUtf8) = 0)
ret++;
VBOX_UTF8_FREE(nameUtf8);
@@ -7585,7 +7585,7 @@ static int
vboxConnectListDefinedNetworks(virConnectPtr conn, char **const names
VBOX_UTF16_TO_UTF8(nameUtf16, nameUtf8);
VIR_DEBUG(nnames[%d]: %s, ret, nameUtf8);
-if (VIR_STRDUP(names[ret], nameUtf8) == 0)
+if (VIR_STRDUP(names[ret], nameUtf8) = 0)
ret++;
VBOX_UTF8_FREE(nameUtf8);
@@ -8087,7 +8087,7 @@ static char *vboxNetworkGetXMLDesc(virNetworkPtr
network,
networkInterface-vtbl-GetInterfaceType(networkInterface,
interfaceType);
if (interfaceType == HostNetworkInterfaceType_HostOnly) {
-if (VIR_STRDUP(def-name, network-name) == 0) {
+if (VIR_STRDUP(def-name, network-name) = 0) {
PRUnichar *networkNameUtf16 = NULL;
IDHCPServer *dhcpServer = NULL;
vboxIID vboxnet0IID = VBOX_IID_INITIALIZER;
ACK
Michal
Thanks, pushed now.
Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RESEND 0/5] VirtualBox version 4.2 support for libvirt vbox driver
Hello, I'm re-sending this patch for reviewing. If necessary I'm willing to make some changes to those patches. I'm currently working on a better management for snapshots with virtualbox, and my work is based on Virtualbox 4.2 so that's why I'm re sending this patch. Regards, Manuel VIVES Ryan Woodsmall said originally: This patch set adds VirtualBox 4.2 initial support for the libvirt vbox driver. I've tested enough to check capabilities, create a VM, destroy it, etc. Five patches total: - Patch 1 is the C API header file from Oracle, cleaned up for libvirt. - Patch 2 is the version specific source file for version dependency. - Patch 3 is the src/Makefile.am change to pick up the two new files. - Patch 4 is the vbox driver support for the new VirtualBox API/version. - Patch 5 is the vbox_tmpl.c template support for the new version. A few things have changed in the VirtualBox API - some small (capitalizations of things in function names like Ip to IP and Dhcp to DHCP) and some much larger (FindMedium is superceded by OpenMedium). The biggest change for the sake of this patch set is the signature of CreateMachine is quite a bit different. Using the Oracle source as a guide, to spin up a VM with a given UUID, it looks like a text flag has to be passed in a new argument to CreateMachine. This flag is built in the VirtualBox 4.2 specific ifdefs and is kind of ugly but works. Additionally, there is now (unused) VM groups support in CreateMachine and the previous 'osTypeId' arg is currently set to nsnull as in the Oracle code. The FindMedium to OpenMedium changes were more straightforward and are pretty clear. The rest of the vbox template changes are basically spelling/capitalization changes from the looks of things. This probably isn't perfect, but it works on git and patched against 0.10.2 for a few quick tests. Not currently on the list, so ping me directly if you need any other info on these, or if anything could use additional work. Thanks! ryan woodsmall (5): vbox C API header for VirtualBox v4.2 vbox version-specific C file for VirtualBox v4.2 Makefile.am additions for VirtualBox v4.2 vbox driver support for VirtualBox v4.2 vbox template support for VirtualBox v4.2 src/Makefile.am |3 +- src/vbox/vbox_CAPI_v4_2.h | 8855 + src/vbox/vbox_V4_2.c | 13 + src/vbox/vbox_driver.c|8 + src/vbox/vbox_tmpl.c | 90 +- 5 files changed, 8958 insertions(+), 11 deletions(-) create mode 100644 src/vbox/vbox_CAPI_v4_2.h create mode 100644 src/vbox/vbox_V4_2.c -- 1.7.10.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RESEND 5/5] vbox template support for VirtualBox v4.2
From: ryan woodsmall rwoodsm...@gmail.com
---
src/vbox/vbox_tmpl.c | 90 --
1 file changed, 80 insertions(+), 10 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index d37888c..5bef956 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -72,6 +72,8 @@
# include vbox_CAPI_v4_0.h
#elif VBOX_API_VERSION == 4001
# include vbox_CAPI_v4_1.h
+#elif VBOX_API_VERSION == 4002
+# include vbox_CAPI_v4_2.h
#else
# error Unsupport VBOX_API_VERSION
#endif
@@ -4178,9 +4180,12 @@ vboxAttachDrives(virDomainDefPtr def, vboxGlobalData
*data, IMachine *machine)
continue;
}
-# if VBOX_API_VERSION = 4000
+# if VBOX_API_VERSION = 4000 VBOX_API_VERSION 4002
data-vboxObj-vtbl-FindMedium(data-vboxObj, mediumFileUtf16,
deviceType, medium);
+# elif VBOX_API_VERSION = 4002
+data-vboxObj-vtbl-OpenMedium(data-vboxObj, mediumFileUtf16,
+deviceType, accessMode, PR_FALSE,
medium);
# endif
if (!medium) {
@@ -4920,7 +4925,11 @@ vboxAttachUSB(virDomainDefPtr def, vboxGlobalData *data,
IMachine *machine)
machine-vtbl-GetUSBController(machine, USBController);
if (USBController) {
USBController-vtbl-SetEnabled(USBController, 1);
+#if VBOX_API_VERSION 4002
USBController-vtbl-SetEnabledEhci(USBController, 1);
+#else
+USBController-vtbl-SetEnabledEHCI(USBController, 1);
+#endif
for (i = 0; i def-nhostdevs; i++) {
if (def-hostdevs[i]-mode ==
@@ -5025,10 +5034,18 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr
conn, const char *xml) {
vboxIID mchiid = VBOX_IID_INITIALIZER;
virDomainDefPtr def = NULL;
PRUnichar *machineNameUtf16 = NULL;
-#if VBOX_API_VERSION = 3002
+#if VBOX_API_VERSION = 3002 VBOX_API_VERSION 4002
PRBool override = PR_FALSE;
#endif
nsresult rc;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+#if VBOX_API_VERSION = 4002
+const char *flagsUUIDPrefix = UUID=;
+const char *flagsForceOverwrite = forceOverwrite=0;
+const char *flagsSeparator = ,;
+char createFlags[strlen(flagsUUIDPrefix) + VIR_UUID_STRING_BUFLEN +
strlen(flagsSeparator) + strlen(flagsForceOverwrite) + 1];
+PRUnichar *createFlagsUtf16 = NULL;
+#endif
if (!(def = virDomainDefParseString(xml, data-caps, data-xmlopt,
1 VIR_DOMAIN_VIRT_VBOX,
@@ -5038,6 +5055,8 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr
conn, const char *xml) {
VBOX_UTF8_TO_UTF16(def-name, machineNameUtf16);
vboxIIDFromUUID(iid, def-uuid);
+virUUIDFormat(def-uuid, uuidstr);
+
#if VBOX_API_VERSION 3002
rc = data-vboxObj-vtbl-CreateMachine(data-vboxObj,
machineNameUtf16,
@@ -5053,7 +5072,7 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr
conn, const char *xml) {
iid.value,
override,
machine);
-#else /* VBOX_API_VERSION = 4000 */
+#elif VBOX_API_VERSION = 4000 VBOX_API_VERSION 4002
rc = data-vboxObj-vtbl-CreateMachine(data-vboxObj,
NULL,
machineNameUtf16,
@@ -5061,7 +5080,23 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr
conn, const char *xml) {
iid.value,
override,
machine);
-#endif /* VBOX_API_VERSION = 4000 */
+#else /* VBOX_API_VERSION = 4002 */
+snprintf(createFlags, sizeof(createFlags), %s%s%s%s,
+ flagsUUIDPrefix,
+ uuidstr,
+ flagsSeparator,
+ flagsForceOverwrite
+);
+VBOX_UTF8_TO_UTF16(createFlags, createFlagsUtf16);
+rc = data-vboxObj-vtbl-CreateMachine(data-vboxObj,
+NULL,
+machineNameUtf16,
+0,
+nsnull,
+nsnull,
+createFlagsUtf16,
+machine);
+#endif /* VBOX_API_VERSION = 4002 */
VBOX_UTF16_FREE(machineNameUtf16);
if (NS_FAILED(rc)) {
@@ -7845,15 +7880,26 @@ static virNetworkPtr
vboxNetworkDefineCreateXML(virConnectPtr conn, const char *
* IP and enables the interface so even if the dhcpserver is not
* started the interface is still up and running
*/
+#if VBOX_API_VERSION 4002
networkInterface-vtbl-EnableStaticIpConfig(networkInterface,
[libvirt] [PATCH RESEND 4/5] vbox driver support for VirtualBox v4.2
From: ryan woodsmall rwoodsm...@gmail.com
---
src/vbox/vbox_driver.c |8
1 file changed, 8 insertions(+)
diff --git a/src/vbox/vbox_driver.c b/src/vbox/vbox_driver.c
index a68f33d..9d07574 100644
--- a/src/vbox/vbox_driver.c
+++ b/src/vbox/vbox_driver.c
@@ -61,6 +61,9 @@ extern virStorageDriver vbox40StorageDriver;
extern virDriver vbox41Driver;
extern virNetworkDriver vbox41NetworkDriver;
extern virStorageDriver vbox41StorageDriver;
+extern virDriver vbox42Driver;
+extern virNetworkDriver vbox42NetworkDriver;
+extern virStorageDriver vbox42StorageDriver;
static virDriver vboxDriverDummy;
@@ -124,6 +127,11 @@ int vboxRegister(void) {
driver= vbox41Driver;
networkDriver = vbox41NetworkDriver;
storageDriver = vbox41StorageDriver;
+} else if (uVersion = 4001051 uVersion 4002051) {
+VIR_DEBUG(VirtualBox API version: 4.2);
+driver= vbox42Driver;
+networkDriver = vbox42NetworkDriver;
+storageDriver = vbox42StorageDriver;
} else {
VIR_DEBUG(Unsupported VirtualBox API version: %u, uVersion);
}
--
1.7.10.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RESEND 2/5] vbox version-specific C file for VirtualBox v4.2
From: ryan woodsmall rwoodsm...@gmail.com --- src/vbox/vbox_V4_2.c | 13 + 1 file changed, 13 insertions(+) create mode 100644 src/vbox/vbox_V4_2.c diff --git a/src/vbox/vbox_V4_2.c b/src/vbox/vbox_V4_2.c new file mode 100644 index 000..8c5b61c --- /dev/null +++ b/src/vbox/vbox_V4_2.c @@ -0,0 +1,13 @@ +/** @file vbox_V4_2.c + * C file to include support for multiple versions of VirtualBox + * at runtime. + */ + +#include config.h + +/** The API Version */ +#define VBOX_API_VERSION4002 +/** Version specific prefix. */ +#define NAME(name) vbox42##name + +#include vbox_tmpl.c -- 1.7.10.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RESEND 3/5] Makefile.am additions for VirtualBox v4.2
From: ryan woodsmall rwoodsm...@gmail.com --- src/Makefile.am |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index 430a356..467ec5d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -593,7 +593,8 @@ VBOX_DRIVER_SOURCES = \ vbox/vbox_V3_1.c vbox/vbox_CAPI_v3_1.h \ vbox/vbox_V3_2.c vbox/vbox_CAPI_v3_2.h \ vbox/vbox_V4_0.c vbox/vbox_CAPI_v4_0.h \ - vbox/vbox_V4_1.c vbox/vbox_CAPI_v4_1.h + vbox/vbox_V4_1.c vbox/vbox_CAPI_v4_1.h \ + vbox/vbox_V4_2.c vbox/vbox_CAPI_v4_2.h VBOX_DRIVER_EXTRA_DIST = \ vbox/vbox_tmpl.c vbox/README\ -- 1.7.10.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [test-API][PATCH] Add env check function for memory_params_live
In this case, domain memory cgroup path is hardcoded and fail the
case after cgroup path changed recently. To avoid such failure, add
check function for lscgroup command before run this case, if check
fail then skip this case.
Signed-off-by: Wayne Sun g...@redhat.com
---
repos/domain/memory_params_live.py | 36 ++--
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/repos/domain/memory_params_live.py
b/repos/domain/memory_params_live.py
index 44fb8b4..280792a 100644
--- a/repos/domain/memory_params_live.py
+++ b/repos/domain/memory_params_live.py
@@ -4,6 +4,7 @@
import os
import math
+import commands
from xml.dom import minidom
import libvirt
@@ -15,27 +16,35 @@ required_params = ('guestname', 'hard_limit', 'soft_limit',
'swap_hard_limit', )
optional_params = {}
UNLIMITED = 9007199254740991
-CGROUP_PATH = /cgroup/memory/libvirt/qemu
+CGROUP_PATH = /cgroup/
def get_cgroup_setting(guestname):
get domain memory parameters in cgroup
if os.path.exists(CGROUP_PATH):
-cgroup_path = %s/%s % (CGROUP_PATH, guestname)
+cgroup_path = CGROUP_PATH
else:
-cgroup_path = /sys/fs%s/%s % (CGROUP_PATH, guestname)
+cgroup_path = /sys/fs%s % CGROUP_PATH
-f = open(%s/memory.limit_in_bytes % cgroup_path)
+cmd = lscgroup | grep %s | grep memory: % guestname
+ret, out = commands.getstatusoutput(cmd)
+if ret:
+logger.error(out)
+return 1
+else:
+mem_cgroup_path = %s%s % (cgroup_path, out.replace(':', ''))
+
+f = open(%s/memory.limit_in_bytes % mem_cgroup_path)
hard = int(f.read())
logger.info(memory.limit_in_bytes value is %s % hard)
f.close()
-f = open(%s/memory.soft_limit_in_bytes % cgroup_path)
+f = open(%s/memory.soft_limit_in_bytes % mem_cgroup_path)
soft = int(f.read())
logger.info(memory.soft_limit_in_bytes value is %s % soft)
f.close()
-f = open(%s/memory.memsw.limit_in_bytes % cgroup_path)
+f = open(%s/memory.memsw.limit_in_bytes % mem_cgroup_path)
swap = int(f.read())
logger.info(memory.memsw.limit_in_bytes value is %s % swap)
f.close()
@@ -98,6 +107,10 @@ def memory_params_live(params):
logger.info(check memory parameters in cgroup)
ret = get_cgroup_setting(guestname)
+if ret == 1:
+logger.error(fail to get domain memory cgroup setting)
+return 1
+
for i in param_dict.keys():
if math.fabs(param_dict[i] - ret[i]) 1:
logger.error(%s value not match with cgroup setting % i)
@@ -110,3 +123,14 @@ def memory_params_live(params):
return 1
return 0
+
+def memory_params_live_check(params):
+check lscgroup packages
+
+logger = params['logger']
+cmd = 'lscgroup'
+ret, out = commands.getstatusoutput(cmd)
+if ret and 'command not found' in out:
+logger.error(out)
+logger.error(package libcgroup or libcgroup-tools is not installed)
+return 1
--
1.8.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3 v4] add 'sharePolicy' attribute for qemu vnc sharing policy
On 05/22/2013 01:50 PM, Ján Tomko wrote: On 05/21/2013 04:31 PM, Guannan Ren wrote: v3-v4 add missing .args, .xml files (I raw it in git Untracked files this morning and thought about it for a while, then git-cleaned them...) rebase work. v2-v3 rebase work. v1-v2: changed attribute name from 'policy' to 'sharePolicy' renamed caps flag name: QEMU_CAPS_VNC_SHARE_POLICY fixed issues pointed out in v1 review have to keep hard-coded version probe after checking qemu -help and qmp command. As there were some vnc patches recently, I chose do rebase work a little later. These patches try to add a new attribute 'sharePolicy' to element graphics of vnc type. This attribute has three values: allow-exclusive,force-shared and ignore. ACK series. Thanks for the review. pushed. Guannan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: fix use after free in virChrdevOpen
Don't free the stream on error if we've successfully added it
to the hash table, since it will be freed by virChrdevHashEntryFree
callback.
Preserve the error message before calling virStreamFree, since it
resets the error.
Reported by Sergey Fionov on libvir-list.
---
src/conf/virchrdev.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c
index 025d4a8..879c27c 100644
--- a/src/conf/virchrdev.c
+++ b/src/conf/virchrdev.c
@@ -343,6 +343,8 @@ int virChrdevOpen(virChrdevsPtr devs,
virStreamPtr savedStream;
const char *path;
int ret;
+bool added = false;
+virErrorPtr savedError;
switch (source-type) {
case VIR_DOMAIN_CHR_TYPE_PTY:
@@ -399,6 +401,7 @@ int virChrdevOpen(virChrdevsPtr devs,
if (virHashAddEntry(devs-hash, path, st) 0)
goto error;
+added = true;
cbdata-devs = devs;
if (!(cbdata-path = strdup(path))) {
@@ -433,8 +436,16 @@ int virChrdevOpen(virChrdevsPtr devs,
return 0;
error:
-virStreamFree(st);
-virHashRemoveEntry(devs-hash, path);
+savedError = virSaveLastError();
+
+if (added)
+virHashRemoveEntry(devs-hash, path);
+else
+virStreamFree(st);
+
+virSetError(savedError);
+virFreeError(savedError);
+
if (cbdata)
VIR_FREE(cbdata-path);
VIR_FREE(cbdata);
--
1.8.1.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/12] storage_conf: Remove the useless casting
---
src/conf/storage_conf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 073099b..6f89f1c 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -672,7 +672,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
} else {
virReportError(VIR_ERR_XML_ERROR,
_(unknown auth type '%s'),
- (const char *)authType);
+ authType);
goto cleanup;
}
}
@@ -828,9 +828,9 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
}
type = virXPathString(string(./@type), ctxt);
-if ((ret-type = virStoragePoolTypeFromString((const char *)type)) 0) {
+if ((ret-type = virStoragePoolTypeFromString(type)) 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _(unknown storage pool type %s), (const char*)type);
+ _(unknown storage pool type %s), type);
goto cleanup;
}
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 01/12] storage_conf: Fix the wrong error message
It's for parsing login attribute of auth.
---
src/conf/storage_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 6cb98bf..bd8eef0 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -447,7 +447,7 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt,
auth-login = virXPathString(string(./auth/@login), ctxt);
if (auth-login == NULL) {
virReportError(VIR_ERR_XML_ERROR,
- %s, _(missing auth host attribute));
+ %s, _(missing auth login attribute));
return -1;
}
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 02/12] storage_conf: Don't leak uuid in virStoragePoolDefParseAuthCephx
Any string returned from virXPathString should be freed.
---
src/conf/storage_conf.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index bd8eef0..073099b 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -466,6 +466,8 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
virStoragePoolAuthCephxPtr auth)
{
char *uuid = NULL;
+int ret = -1;
+
auth-username = virXPathString(string(./auth/@username), ctxt);
if (auth-username == NULL) {
virReportError(VIR_ERR_XML_ERROR,
@@ -485,19 +487,22 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
if (auth-secret.usage != NULL) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(either auth secret uuid or usage expected));
-return -1;
+goto cleanup;
}
if (virUUIDParse(uuid, auth-secret.uuid) 0) {
virReportError(VIR_ERR_XML_ERROR,
%s, _(invalid auth secret uuid));
-return -1;
+goto cleanup;
}
auth-secret.uuidUsable = true;
} else {
auth-secret.uuidUsable = false;
}
-return 0;
+ret = 0;
+cleanup:
+VIR_FREE(uuid);
+return ret;
}
static int
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 04/12] storage_conf: Use xmlStrEqual instead of STREQ
And improve the error message
---
src/conf/storage_conf.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 6f89f1c..5c8577e 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -975,9 +975,11 @@ virStoragePoolDefParseNode(xmlDocPtr xml,
xmlXPathContextPtr ctxt = NULL;
virStoragePoolDefPtr def = NULL;
-if (STRNEQ((const char *)root-name, pool)) {
+if (!xmlStrEqual(root-name, BAD_CAST pool)) {
virReportError(VIR_ERR_XML_ERROR,
- %s, _(unknown root element for storage pool));
+ _(unexpected root element %s,
+ expecting pool),
+ root-name);
goto cleanup;
}
@@ -1353,9 +1355,11 @@ virStorageVolDefParseNode(virStoragePoolDefPtr pool,
xmlXPathContextPtr ctxt = NULL;
virStorageVolDefPtr def = NULL;
-if (STRNEQ((const char *)root-name, volume)) {
+if (!xmlStrEqual(root-name, BAD_CAST volume)) {
virReportError(VIR_ERR_XML_ERROR,
- %s, _(unknown root element for storage vol));
+ _(unexpected root element %s,
+ expecting volume),
+ root-name);
goto cleanup;
}
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 07/12] storage_conf: Improve the memory deallocation of pool def parsing
Changes:
* Free all the strings at cleanup, instead of freeing them
in the middle
* Remove xmlFree
* s/tmppath/target_path/, to make it more sensible
* Add new goto label error
---
src/conf/storage_conf.c | 54 -
1 file changed, 26 insertions(+), 28 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index efe02e8..6f0ed74 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -820,7 +820,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
xmlNodePtr source_node;
char *type = NULL;
char *uuid = NULL;
-char *tmppath;
+char *target_path = NULL;
if (VIR_ALLOC(ret) 0) {
virReportOOMError();
@@ -831,21 +831,18 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
if ((ret-type = virStoragePoolTypeFromString(type)) 0) {
virReportError(VIR_ERR_XML_ERROR,
_(unknown storage pool type %s), type);
-goto cleanup;
+goto error;
}
-xmlFree(type);
-type = NULL;
-
if ((options = virStoragePoolOptionsForPoolType(ret-type)) == NULL) {
-goto cleanup;
+goto error;
}
source_node = virXPathNode(./source, ctxt);
if (source_node) {
if (virStoragePoolDefParseSource(ctxt, ret-source, ret-type,
source_node) 0)
-goto cleanup;
+goto error;
}
ret-name = virXPathString(string(./name), ctxt);
@@ -855,7 +852,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
if (ret-name == NULL) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(missing pool source name element));
-goto cleanup;
+goto error;
}
uuid = virXPathString(string(./uuid), ctxt);
@@ -863,22 +860,21 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
if (virUUIDGenerate(ret-uuid) 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, %s,
_(unable to generate uuid));
-goto cleanup;
+goto error;
}
} else {
if (virUUIDParse(uuid, ret-uuid) 0) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed uuid element));
-goto cleanup;
+goto error;
}
-VIR_FREE(uuid);
}
if (options-flags VIR_STORAGE_POOL_SOURCE_HOST) {
if (!ret-source.nhost) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source host name));
-goto cleanup;
+goto error;
}
}
@@ -886,7 +882,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
if (!ret-source.dir) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source path));
-goto cleanup;
+goto error;
}
}
if (options-flags VIR_STORAGE_POOL_SOURCE_NAME) {
@@ -895,7 +891,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
ret-source.name = strdup(ret-name);
if (ret-source.name == NULL) {
virReportOOMError();
-goto cleanup;
+goto error;
}
}
}
@@ -904,7 +900,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
if (!ret-source.adapter.type) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source adapter));
-goto cleanup;
+goto error;
}
if (ret-source.adapter.type ==
@@ -914,7 +910,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
virReportError(VIR_ERR_XML_ERROR, %s,
_('wwnn' and 'wwpn' must be specified for
adapter
type 'fchost'));
-goto cleanup;
+goto error;
}
if (!virValidateWWN(ret-source.adapter.data.fchost.wwnn) ||
@@ -925,7 +921,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
if (!ret-source.adapter.data.name) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source adapter name));
-goto cleanup;
+goto error;
}
}
}
@@ -935,36 +931,38 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
if (!ret-source.ndevice) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source device name));
-goto cleanup;
+goto error;
}
}
/* When we are working with a virtual disk we can skip the target
* path and permissions */
if (!(options-flags VIR_STORAGE_POOL_SOURCE_NETWORK)) {
-if ((tmppath = virXPathString(string(./target/path), ctxt)) == NULL)
{
+if
[libvirt] [PATCH 08/12] storage_conf: Improve the memory deallocation of virStorageVolDefParseXML
Changes:
* Add a new goto label error
* Free the strings at cleanup
* Remove the unnecessary frees
---
src/conf/storage_conf.c | 29 ++---
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 6f0ed74..4c08cea 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1253,7 +1253,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
if (ret-name == NULL) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(missing volume name element));
-goto cleanup;
+goto error;
}
/* Auto-generated so deliberately ignore */
@@ -1264,20 +1264,17 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
if (capacity == NULL) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(missing capacity element));
-goto cleanup;
+goto error;
}
if (virStorageSize(unit, capacity, ret-capacity) 0)
-goto cleanup;
-VIR_FREE(capacity);
+goto error;
VIR_FREE(unit);
allocation = virXPathString(string(./allocation), ctxt);
if (allocation) {
unit = virXPathString(string(./allocation/@unit), ctxt);
if (virStorageSize(unit, allocation, ret-allocation) 0)
-goto cleanup;
-VIR_FREE(allocation);
-VIR_FREE(unit);
+goto error;
} else {
ret-allocation = ret-capacity;
}
@@ -1294,7 +1291,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
virReportError(VIR_ERR_XML_ERROR,
_(unknown volume format type %s), format);
VIR_FREE(format);
-goto cleanup;
+goto error;
}
VIR_FREE(format);
}
@@ -1302,14 +1299,14 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
if (virStorageDefParsePerms(ctxt, ret-target.perms,
./target/permissions,
DEFAULT_VOL_PERM_MODE) 0)
-goto cleanup;
+goto error;
node = virXPathNode(./target/encryption, ctxt);
if (node != NULL) {
ret-target.encryption = virStorageEncryptionParseNode(ctxt-doc,
node);
if (ret-target.encryption == NULL)
-goto cleanup;
+goto error;
}
ret-backingStore.path = virXPathString(string(./backingStore/path),
ctxt);
@@ -1324,7 +1321,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
virReportError(VIR_ERR_XML_ERROR,
_(unknown volume format type %s), format);
VIR_FREE(format);
-goto cleanup;
+goto error;
}
VIR_FREE(format);
}
@@ -1332,16 +1329,18 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
if (virStorageDefParsePerms(ctxt, ret-backingStore.perms,
./backingStore/permissions,
DEFAULT_VOL_PERM_MODE) 0)
-goto cleanup;
-
-return ret;
+goto error;
cleanup:
VIR_FREE(allocation);
VIR_FREE(capacity);
VIR_FREE(unit);
+return ret;
+
+error:
virStorageVolDefFree(ret);
-return NULL;
+ret = NULL;
+goto cleanup;
}
virStorageVolDefPtr
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 05/12] storage_conf: Put %s at the same line with error type
Trivial, but it allows the error message to have more spaces.
---
src/conf/storage_conf.c | 95 -
1 file changed, 47 insertions(+), 48 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5c8577e..76dae52 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -446,15 +446,15 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt,
{
auth-login = virXPathString(string(./auth/@login), ctxt);
if (auth-login == NULL) {
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(missing auth login attribute));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(missing auth login attribute));
return -1;
}
auth-passwd = virXPathString(string(./auth/@passwd), ctxt);
if (auth-passwd == NULL) {
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(missing auth passwd attribute));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(missing auth passwd attribute));
return -1;
}
@@ -470,8 +470,8 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
auth-username = virXPathString(string(./auth/@username), ctxt);
if (auth-username == NULL) {
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(missing auth username attribute));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(missing auth username attribute));
return -1;
}
@@ -490,8 +490,8 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
goto cleanup;
}
if (virUUIDParse(uuid, auth-secret.uuid) 0) {
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(invalid auth secret uuid));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(invalid auth secret uuid));
goto cleanup;
}
auth-secret.uuidUsable = true;
@@ -564,8 +564,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
for (i = 0; i source-nhost; i++) {
name = virXMLPropString(nodeset[i], name);
if (name == NULL) {
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(missing storage pool host name));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(missing storage pool host name));
goto cleanup;
}
source-hosts[i].name = name;
@@ -600,8 +600,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
char *path = virXMLPropString(nodeset[i], path);
if (path == NULL) {
VIR_FREE(nodeset);
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(missing storage pool source device
path));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(missing storage pool source device path));
goto cleanup;
}
source-devices[i].path = path;
@@ -773,8 +773,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
if (virStrToLong_i(mode, NULL, 8, tmp) 0 || (tmp ~0777)) {
VIR_FREE(mode);
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(malformed octal mode));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(malformed octal mode));
goto error;
}
perms-mode = tmp;
@@ -785,8 +785,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
perms-uid = (uid_t) -1;
} else {
if (virXPathLong(number(./owner), ctxt, v) 0) {
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(malformed owner element));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(malformed owner element));
goto error;
}
perms-uid = (int)v;
@@ -796,8 +796,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
perms-gid = (gid_t) -1;
} else {
if (virXPathLong(number(./group), ctxt, v) 0) {
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(malformed group element));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(malformed group element));
goto error;
}
perms-gid = (int)v;
@@ -853,22 +853,22 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
options-flags VIR_STORAGE_POOL_SOURCE_NAME)
ret-name = ret-source.name;
if (ret-name == NULL) {
-virReportError(VIR_ERR_XML_ERROR,
- %s, _(missing pool source name element));
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(missing pool source name element));
goto cleanup;
}
uuid = virXPathString(string(./uuid), ctxt);
if (uuid == NULL) {
if
[libvirt] [PATCH 10/12] storage_conf: Use VIR_STRDUP instead of strdup
---
src/conf/storage_conf.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 1f376ef..44ecb2a 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -888,11 +888,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
if (options-flags VIR_STORAGE_POOL_SOURCE_NAME) {
if (ret-source.name == NULL) {
/* source name defaults to pool name */
-ret-source.name = strdup(ret-name);
-if (ret-source.name == NULL) {
-virReportOOMError();
+if (VIR_STRDUP(ret-source.name, ret-name) 0)
goto error;
-}
}
}
@@ -1710,16 +1707,12 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
}
VIR_FREE(pool-configFile); /* for driver reload */
-pool-configFile = strdup(path);
-if (pool-configFile == NULL) {
-virReportOOMError();
+if (VIR_STRDUP(pool-configFile, path) 0) {
virStoragePoolDefFree(def);
return NULL;
}
VIR_FREE(pool-autostartLink); /* for driver reload */
-pool-autostartLink = strdup(autostartLink);
-if (pool-autostartLink == NULL) {
-virReportOOMError();
+if (VIR_STRDUP(pool-autostartLink, autostartLink) 0) {
virStoragePoolDefFree(def);
return NULL;
}
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 12/12] storage_conf: Use uid_t/gid_t instead of int to cast the value
And error out if the casted value is not same with the original
one, which prevents the bug on platform(s) where uid_t/gid_t
has different size with long.
---
src/conf/storage_conf.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index a62629e..a648c6d 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -747,7 +747,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
int defaultmode)
{
char *mode;
-long v;
+long val;
int ret = -1;
xmlNodePtr relnode;
xmlNodePtr node;
@@ -784,23 +784,26 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
if (virXPathNode(./owner, ctxt) == NULL) {
perms-uid = (uid_t) -1;
} else {
-if (virXPathLong(number(./owner), ctxt, v) 0) {
+if (virXPathLong(number(./owner), ctxt, val) 0 ||
+(uid_t)val != val) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed owner element));
goto error;
}
-perms-uid = (int)v;
+
+perms-uid = (uid_t)val;
}
if (virXPathNode(./group, ctxt) == NULL) {
perms-gid = (gid_t) -1;
} else {
-if (virXPathLong(number(./group), ctxt, v) 0) {
+if (virXPathLong(number(./group), ctxt, val) 0 ||
+(gid_t)val != val) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed group element));
goto error;
}
-perms-gid = (int)v;
+perms-gid = (gid_t)val;
}
/* NB, we're ignoring missing labels here - they'll simply inherit */
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 06/12] storage_conf: Fix the error type
s/VIR_ERR_INTERNAL_ERROR/VIR_ERR_XML_ERROR/.
---
src/conf/storage_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 76dae52..efe02e8 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -829,7 +829,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
type = virXPathString(string(./@type), ctxt);
if ((ret-type = virStoragePoolTypeFromString(type)) 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
+virReportError(VIR_ERR_XML_ERROR,
_(unknown storage pool type %s), type);
goto cleanup;
}
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 00/12 v2] storage_conf: left cleanup/fixes/improvement patches
Part of v1 are pushed, the left patches are splitted Osier Yang (12): storage_conf: Fix the wrong error message storage_conf: Don't leak uuid in virStoragePoolDefParseAuthCephx storage_conf: Remove the useless casting storage_conf: Use xmlStrEqual instead of STREQ storage_conf: Put %s at the same line with error type storage_conf: Fix the error type storage_conf: Improve the memory deallocation of pool def parsing storage_conf: Improve the memory deallocation of virStorageVolDefParseXML storage_conf: Use NULLSTR instead storage_conf: Use VIR_STRDUP instead of strdup storage_conf: Improve error messages storage_conf: Use uid_t/gid_t instead of int to cast the value src/conf/storage_conf.c | 242 1 file changed, 122 insertions(+), 120 deletions(-) -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 09/12] storage_conf: Use NULLSTR instead
---
src/conf/storage_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 4c08cea..1f376ef 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1485,7 +1485,7 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool,
virBufferAddLit(buf, volume\n);
virBufferAsprintf(buf, name%s/name\n, def-name);
-virBufferAsprintf(buf, key%s/key\n, def-key ? def-key :
(null));
+virBufferAsprintf(buf, key%s/key\n, NULLSTR(def-key));
virBufferAddLit(buf, source\n);
if (def-source.nextent) {
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 11/12] storage_conf: Improve error messages
virStoragePoolDefParseSource:
* Better error message
virStoragePoolObjLoad:
* Break the line line
---
src/conf/storage_conf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 44ecb2a..a62629e 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -531,7 +531,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
source-name = virXPathString(string(./name), ctxt);
if (pool_type == VIR_STORAGE_POOL_RBD source-name == NULL) {
virReportError(VIR_ERR_XML_ERROR, %s,
- _(missing mandatory 'name' field for RBD pool name));
+ _(element 'name' is mandatory for RBD pool name));
goto cleanup;
}
@@ -1695,7 +1695,8 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
if (!virFileMatchesNameSuffix(file, def-name, .xml)) {
virReportError(VIR_ERR_XML_ERROR,
- _(Storage pool config filename '%s' does not match
pool name '%s'),
+ _(Storage pool config filename '%s' does
+ not match pool name '%s'),
path, def-name);
virStoragePoolDefFree(def);
return NULL;
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/12] storage_conf: Use uid_t/gid_t instead of int to cast the value
On 22/05/13 20:05, Osier Yang wrote:
And error out if the casted value is not same with the original
one, which prevents the bug on platform(s) where uid_t/gid_t
has different size with long.
---
src/conf/storage_conf.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index a62629e..a648c6d 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -747,7 +747,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
int defaultmode)
{
char *mode;
-long v;
+long val;
int ret = -1;
xmlNodePtr relnode;
xmlNodePtr node;
@@ -784,23 +784,26 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
if (virXPathNode(./owner, ctxt) == NULL) {
perms-uid = (uid_t) -1;
} else {
-if (virXPathLong(number(./owner), ctxt, v) 0) {
+if (virXPathLong(number(./owner), ctxt, val) 0 ||
+(uid_t)val != val) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed owner element));
goto error;
}
-perms-uid = (int)v;
+
+perms-uid = (uid_t)val;
}
if (virXPathNode(./group, ctxt) == NULL) {
perms-gid = (gid_t) -1;
} else {
-if (virXPathLong(number(./group), ctxt, v) 0) {
+if (virXPathLong(number(./group), ctxt, val) 0 ||
+(gid_t)val != val) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed group element));
goto error;
}
-perms-gid = (int)v;
+perms-gid = (gid_t)val;
}
/* NB, we're ignoring missing labels here - they'll simply inherit */
Rng schema for owner, and group:
element name='owner'
choice
ref name='unsignedInt'/
value-1/value
/choice
/element
element name='group'
choice
ref name='unsignedInt'/
value-1/value
/choice
/element
Which means the patch has to filter out the -1 when do
(uid_t)val != val or (gid_t)val != val.
So with the attached diff squashed in (will update the commit
log when pushing)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index a648c6d..082296a 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -44,6 +44,7 @@
#include viralloc.h
#include virfile.h
#include virstring.h
+#include virlog.h
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -785,7 +786,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
perms-uid = (uid_t) -1;
} else {
if (virXPathLong(number(./owner), ctxt, val) 0 ||
-(uid_t)val != val) {
+((uid_t)val != val
+ val != -1)) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed owner element));
goto error;
@@ -798,7 +800,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
perms-gid = (gid_t) -1;
} else {
if (virXPathLong(number(./group), ctxt, val) 0 ||
-(gid_t)val != val) {
+((gid_t) val != val
+ val != -1)) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed group element));
goto error;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [virt-tools-list] Q on virsh attach-disk , why no network disk ?
On 05/22/2013 08:55 AM, Deepak C Shetty wrote: Hi All, I was looking at virsh attach-disk and i see the below [snip] DESCRIPTION Attach new disk device. OPTIONS [--domain] string domain name, id or uuid [--source] string source of disk device [--target] string target of disk device ... ... *--sourcetype string type of source (block|file)* [/snip] I was wondering why the sourcetype only support block|file only ? What if i wanted to add a network (disk type=network) based drive ? Is this a constraint of virsh or QEMU itself doesn't allow hotplu of network block device disk ? virsh questions go to libvirt-list, CC'd. It's just a missing feature, not a deliberate omission. Patches welcome. - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RESENT 01/12] libxl: allow script for any network interface, not only bridge
Jim Fehlig wrote: Jim Fehlig wrote: Laine Stump wrote: On 04/10/2013 05:10 AM, Daniel P. Berrange wrote: On Wed, Apr 10, 2013 at 04:44:43AM +0200, Marek Marczykowski wrote: This can be useful for route or NAT networks, or any other custom network setup. Especially configuration example in documentation uses script/ tag with type 'ethernet'. --- src/libxl/libxl_conf.c | 17 + 1 file changed, 5 insertions(+), 12 deletions(-) The script should not not have been allowed for type='bridge' in the first place, it is only intended for type='ethernet' usage and nothing else. I thought that it was also allowed/necessary for type='bridge' in xen domains. Is this incorrect? I think it is only necessary if something other than the default (/etc/xen/scripts/vif-bridge) is desired. It has been allowed in both xen drivers for as long as I remember. Any consensus here? I double-checked the legacy xen driver and it does in fact support script for interface type='bridge'. I'm not sure how many users specify something other than the default, but I'm loath to break them when moving from the old xen toolstack to libxl. Marek, Assuming we continue to allow script for type='bridge' interfaces in the libxl driver, you'll have to change your patch to only allow it for type 'bridge' and 'ethernet'. Please send a V2 with this change. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Peter Maydell peter.mayd...@linaro.org writes: On 22 May 2013 14:15, Anthony Liguori aligu...@us.ibm.com wrote: Paolo Bonzini pbonz...@redhat.com writes: You don't need to know what targets were supported in the version that you compiled from. Only one target is supported in this executable anyway. It seems useful to me. One day we may support multiple targets per executable. Why would you care about which architectures the executable supports? What you actually want to know is which machine models are supported; whether board foo happens to be ARM or PPC isn't really very interesting IMHO. That's a very good point. It was the libvirt folks that requested this. Perhaps they can shed some light on the logic? Regards, Anthony Liguori -- PMM -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RESENT 02/12] libxl: PCI passthrough support
Jim Fehlig wrote: Daniel P. Berrange wrote: On Wed, Apr 10, 2013 at 04:44:43AM +0200, Marek Marczykowski wrote: --- src/libxl/libxl_conf.c | 72 ++ src/libxl/libxl_conf.h | 2 ++ 2 files changed, 74 insertions(+) This needs todo more than just create the config. You need to use the libvirt PCI APIs to validate that it is safe to assign the devices requested, and track which devices are given to which guest, to avoid concurrent assignemnt to 2 guests. Basically I'd expect this to borrow all the code currently in the QMEU driver related to the 'activePciHostdevs' and 'inactivePciHostdevs' data structures. Right. Chunyan already posted such a patch series [1], but Laine noted that it would be useful to maintain the state of PCI device assignment for coordination among multiple drivers. Chunyan posted a design [2] for a hostdev passthrough driver, and IIRC she has started coding after receiving feedback from the list. I see that Chunyan has now posted patches for this work https://www.redhat.com/archives/libvir-list/2013-May/msg01162.html Could you can help review and test those? Regards, Jim Regards, Jim [1] https://www.redhat.com/archives/libvir-list/2013-January/msg00697.html [2] https://www.redhat.com/archives/libvir-list/2013-March/msg01331.html -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RESENT 03/12] libxl: nodeDevice* support for PCI devices
Marek Marczykowski wrote:
For now only for PCI devices. Mostly copy-paste from old xen driver.
This one is (or will be) covered by Chanyan's work as well right?
Regards,
Jim
---
src/libxl/libxl_driver.c | 193
+++
1 file changed, 193 insertions(+)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 40a7a6b..011edf8 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -42,6 +42,7 @@
#include libxl.h
#include libxl_driver.h
#include libxl_conf.h
+#include node_device_conf.h
#include xen_xm.h
#include virtypedparam.h
#include viruri.h
@@ -3666,6 +3667,195 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const
char *xml,
return libxlDomainModifyDeviceFlags(dom, xml, flags,
LIBXL_DEVICE_UPDATE);
}
+static int
+libxlNodeDeviceGetPciInfo(virNodeDevicePtr dev,
+ unsigned *domain,
+ unsigned *bus,
+ unsigned *slot,
+ unsigned *function)
+{
+virNodeDeviceDefPtr def = NULL;
+virNodeDevCapsDefPtr cap;
+char *xml = NULL;
+int ret = -1;
+
+xml = virNodeDeviceGetXMLDesc(dev, 0);
+if (!xml)
+goto out;
+
+def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
+if (!def)
+goto out;
+
+cap = def-caps;
+while (cap) {
+if (cap-type == VIR_NODE_DEV_CAP_PCI_DEV) {
+*domain = cap-data.pci_dev.domain;
+*bus = cap-data.pci_dev.bus;
+*slot = cap-data.pci_dev.slot;
+*function = cap-data.pci_dev.function;
+break;
+}
+
+cap = cap-next;
+}
+
+if (!cap) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _(device %s is not a PCI device), dev-name);
+goto out;
+}
+
+ret = 0;
+out:
+virNodeDeviceDefFree(def);
+VIR_FREE(xml);
+return ret;
+}
+
+static int
+libxlNodeDeviceDettach(virNodeDevicePtr dev)
+{
+virPCIDevicePtr pci;
+unsigned domain, bus, slot, function;
+int ret = -1;
+
+if (libxlNodeDeviceGetPciInfo(dev, domain, bus, slot, function) 0)
+return -1;
+
+pci = virPCIDeviceNew(domain, bus, slot, function);
+if (!pci)
+return -1;
+
+if (virPCIDeviceDetach(pci, NULL, NULL, pciback) 0)
+goto out;
+
+ret = 0;
+out:
+virPCIDeviceFree(pci);
+return ret;
+}
+
+static int
+libxlNodeDeviceAssignedDomainId(virNodeDevicePtr dev)
+{
+int numdomains;
+int numpcidevs;
+int ret = -1, i, j;
+int *ids = NULL;
+char *bdf = NULL;
+char *xref = NULL;
+unsigned int domain, bus, slot, function;
+libxl_device_pci *pcidevs = NULL;
+virConnectPtr conn = dev-conn;
+libxlDriverPrivatePtr driver = conn-privateData;
+
+/* Get active domains */
+numdomains = libxlNumDomains(conn);
+if (numdomains 0) {
+return ret;
+}
+if (numdomains 0){
+if (VIR_ALLOC_N(ids, numdomains) 0) {
+virReportOOMError();
+goto out;
+}
+if ((numdomains = libxlListDomains(conn, ids[0], numdomains)) 0) {
+goto out;
+}
+}
+
+/* Get pci bdf */
+if (libxlNodeDeviceGetPciInfo(dev, domain, bus, slot, function) 0)
+goto out;
+
+if (virAsprintf(bdf, %04x:%02x:%02x.%0x,
+domain, bus, slot, function) 0) {
+virReportOOMError();
+goto out;
+}
+
+libxlDriverLock(driver);
+/* Check if bdf is assigned to one of active domains */
+for (i = 0; i numdomains; i++) {
+pcidevs = libxl_device_pci_list(driver-ctx, ids[i], numpcidevs);
+if (pcidevs == NULL) {
+continue;
+} else {
+for (j = 0; j numpcidevs; j++) {
+if (pcidevs[j].bus == bus pcidevs[j].dev == slot
pcidevs[j].func == function) {
+ret = ids[i];
+break;
+}
+}
+VIR_FREE(pcidevs);
+}
+}
+libxlDriverUnlock(driver);
+
+VIR_FREE(xref);
+VIR_FREE(bdf);
+out:
+VIR_FREE(ids);
+
+return ret;
+}
+
+static int
+libxlNodeDeviceReAttach(virNodeDevicePtr dev)
+{
+virPCIDevicePtr pci;
+unsigned domain, bus, slot, function;
+int ret = -1;
+int domid;
+
+if (libxlNodeDeviceGetPciInfo(dev, domain, bus, slot, function) 0)
+return -1;
+
+pci = virPCIDeviceNew(domain, bus, slot, function);
+if (!pci)
+return -1;
+
+/* Check if device is assigned to an active guest */
+if ((domid = libxlNodeDeviceAssignedDomainId(dev)) = 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _(Device %s has been assigned to guest %d),
+
Re: [libvirt] [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Il 22/05/2013 16:29, Anthony Liguori ha scritto: Peter Maydell peter.mayd...@linaro.org writes: On 22 May 2013 14:15, Anthony Liguori aligu...@us.ibm.com wrote: Paolo Bonzini pbonz...@redhat.com writes: You don't need to know what targets were supported in the version that you compiled from. Only one target is supported in this executable anyway. It seems useful to me. One day we may support multiple targets per executable. Why would you care about which architectures the executable supports? What you actually want to know is which machine models are supported; whether board foo happens to be ARM or PPC isn't really very interesting IMHO. That's a very good point. It was the libvirt folks that requested this. Perhaps they can shed some light on the logic? There is processor-dependent logic in libvirt, for example the CPUID bits. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: fix use after free in virChrdevOpen
On 05/22/2013 05:37 AM, Ján Tomko wrote: Don't free the stream on error if we've successfully added it to the hash table, since it will be freed by virChrdevHashEntryFree callback. Preserve the error message before calling virStreamFree, since it resets the error. Reported by Sergey Fionov on libvir-list. --- src/conf/virchrdev.c | 15 +-- 1 file changed, 13 insertions(+), 2 deletions(-) ACK. It might help if you track down which commit introduced the problem and mention that in the commit message. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH RESENT 04/12] libxl: populate xenstore memory entries at startup
Marek Marczykowski wrote:
On 19.04.2013 13:10, Stefano Stabellini wrote:
On Thu, 11 Apr 2013, Marek Marczykowski wrote:
On 11.04.2013 09:52, Ian Campbell wrote:
On Thu, 2013-04-11 at 05:09 +0100, Jim Fehlig wrote:
+/* This will fill xenstore info about free and dom0 memory - if
missing,
+ * should be called before starting first domain */
+if (libxl_get_free_memory(libxl_driver-ctx, free_mem)) {
+VIR_ERROR(_(cannot get free memory info));
+goto error;
+}
Should failure of libxl_get_free_memory() really be fatal and prevent
the driver from loading?
I'm not sure it is intended to be called like this...
I think it is intended to be called as part of starting every domain, to
check if there is enough free memory for that domain, rather than
calling it once at start of day.
In that context if it fails or returns less than the required amount of
memory then that would be fatal for starting that domain.
In xl we use this as part of the auto balloon of dom0, see
xl_cmdimplg.c:freemem. Does libvirt do autoballooning or does it require
dom0_mem? Perhaps this is handled at a higher level?
The problem is how libxl set initial value for freemem-slack. If, for any
reason, dom0 hasn't (almost) all memory assigned before creating first
domain,
15% of host memory will no longer be used at all. This any reason can be
dom0_mem, which is covered by auto value for autoballoon in xen-unstable
(actually only for xl, not libxl in general).
This is the (in)famous incompatibility between autoballoon and dom0_mem.
But this can also happen if
somebody calls xl set-mem 0 some value. The later case doesn't mean the
user
want to disable autoballoon completely.
Calling xl set-mem 0 manually should be compatible with
autoballoon.
Unless it is called before first domain start (which case is covered by my
patch).
However it wouldn't work with dom0_mem.
And to answer you question - libvirt rely on libxl autoballoon.
Could we introduce something similar to autoballoon=auto to libvirt?
Maybe we should push down the autoballoon option to libxl: we should
probably rename autoballoon to libxl_memory_management, enable it
automatically during initialization (and call
libxl__fill_dom0_memory_info) unless dom0_mem has been passed as an
option, in which case we would disable it. If we do it at the libxl
level we would cover xl as well as libvirt and also fix the xl set-mem
0 case.
Looks good for me, but IIUC it's too late for such change in 4.3 and it
doesn't qualify for later backport, right? If so, some approach still is
needed in libvirt for xen 4.2 and 4.3 (like my simple patch). This still
doesn't make libvirt compatible with dom0_mem, but at least cover one
(independent) case. Perhaps additionally autoballoon=auto code should be
copied from xl to libvirt to cover dom0_mem case with xen 4.2 and 4.3?
After reading this thread again, it sounds like the best option is to
support the autoballoon option in libvirt, e.g. in
/etc/libvirt/libxl.conf. (We currently don't have a config file for the
libxl driver, but I think we'll need one anyhow for other knobs, similar
to qemu.conf.) As you note, even if autoballoon is pushed down to
libxl, libvirt would need to handle it for older libxl. And libvirt
needs to handle the dom0_mem case...
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RESENT 07/12] conf: support backend domain name in disk and network devices
Marek Marczykowski wrote:
At least Xen supports backend drivers in another domain (aka driver
domain). This patch introduces XML config option for such setting as
'domain' element with 'name' attribute. Verification its content is left
for the driver.
In the future some option will be needed for USB devices (hostdev
objects), but for now libxl doesn't have support for PVUSB.
---
docs/schemas/domaincommon.rng | 14 ++
src/conf/domain_conf.c| 27 +++
src/conf/domain_conf.h| 2 ++
3 files changed, 43 insertions(+)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 8d7e6db..1423187 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -869,6 +869,13 @@
/optional
ref name=target/
optional
+element name=domain
+ attribute name=name
+ref name=domainName/
+ /attribute
+/element
+ /optional
+ optional
ref name=deviceBoot/
/optional
optional
@@ -1834,6 +1841,13 @@
/element
/optional
optional
+element name=domain
+ attribute name=name
+ref name=domainName/
+ /attribute
+/element
+ /optional
+ optional
I'm certainly no expert in RNG, but do these need to include empty/?
BTW, you'll also need to document this in docs/formatdomain.html.in.
element name=model
attribute name=type
data type=string
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 257a265..bf1fec6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1103,6 +1103,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
VIR_FREE(def-vendor);
VIR_FREE(def-product);
VIR_FREE(def-script);
+VIR_FREE(def-domain_name);
if (def-auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE)
VIR_FREE(def-auth.secret.usage);
virStorageEncryptionFree(def-encryption);
@@ -1228,6 +1229,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
VIR_FREE(def-virtPortProfile);
VIR_FREE(def-script);
+VIR_FREE(def-domain_name);
VIR_FREE(def-ifname);
virDomainDeviceInfoClear(def-info);
@@ -3995,6 +3997,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
char *vendor = NULL;
char *product = NULL;
char *script = NULL;
+char *domain_name = NULL;
int expected_secret_usage = -1;
int auth_secret_usage = -1;
@@ -4153,6 +4156,9 @@ virDomainDiskDefParseXML(virCapsPtr caps,
} else if (!script
xmlStrEqual(cur-name, BAD_CAST script)) {
script = virXMLPropString(cur, path);
+} else if (!domain_name
+ xmlStrEqual(cur-name, BAD_CAST domain)) {
+domain_name = virXMLPropString(cur, name);
} else if (xmlStrEqual(cur-name, BAD_CAST geometry)) {
if (virXPathUInt(string(./geometry/@cyls),
ctxt, def-geometry.cylinders) 0) {
@@ -4447,6 +4453,11 @@ virDomainDiskDefParseXML(virCapsPtr caps,
ctxt-node = saved_node;
}
+if (domain_name != NULL) {
+def-domain_name = domain_name;
+domain_name = NULL;
+}
+
Is the 'if' necessary here? It looks like the other fields of def are
unconditionally assigned, e.g.
def-src = source;
source = NULL;
if (target == NULL) {
virReportError(VIR_ERR_NO_TARGET,
source ? %s : NULL, source);
@@ -4796,6 +4807,7 @@ cleanup:
VIR_FREE(vendor);
VIR_FREE(product);
VIR_FREE(script);
+VIR_FREE(domain_name);
ctxt-node = save_ctxt;
return def;
@@ -5353,6 +5365,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
char *mode = NULL;
char *linkstate = NULL;
char *addrtype = NULL;
+char *domain_name = NULL;
virNWFilterHashTablePtr filterparams = NULL;
virDomainActualNetDefPtr actual = NULL;
xmlNodePtr oldnode = ctxt-node;
@@ -5451,6 +5464,9 @@ virDomainNetDefParseXML(virCapsPtr caps,
} else if (!script
xmlStrEqual(cur-name, BAD_CAST script)) {
script = virXMLPropString(cur, path);
+} else if (!domain_name
+ xmlStrEqual(cur-name, BAD_CAST domain)) {
+domain_name = virXMLPropString(cur, name);
} else if (xmlStrEqual(cur-name, BAD_CAST model)) {
model = virXMLPropString(cur, type);
} else if (xmlStrEqual(cur-name, BAD_CAST driver)) {
@@ -5682,6 +5698,10 @@ virDomainNetDefParseXML(virCapsPtr caps,
def-script = script;
script = NULL;
}
+if (domain_name != NULL) {
+def-domain_name = domain_name;
+domain_name = NULL;
+}
The pattern
Re: [libvirt] [Qemu-devel] [qemu-devel] Default machine type setting for ppc64
On 2013年05月22日 04:01, Anthony Liguori wrote: Daniel P. Berrange berra...@redhat.com writes: On Tue, May 21, 2013 at 11:12:26AM -0600, Eric Blake wrote: I have also argued in the past that it would be useful for libvirt to support the idea of a template, where you can specify a domain XML that inherits defaults from the template. We've already done things like this for networking, nwfilter, and even secret management (in domain XML, you declare that you are using a named network object, and that network object serves as the template instead of you having to hard-code all the elements into your domain XML), so we have a design to base it on. But until someone adds such a feature for libvirt, then OpenStack should be passing explicit XML to libvirt, and tracking defaults at the OpenStack layer. I don't think the idea of a template belongs in libvirt. This is fine. But the conversation started with a statement that it's QEMU's job to define reasonable defaults and libvirt just exposes those. But in QEMU, we punt this problem by letting a user globally override this default. libvirt hides this ability from the end user. So either it's libvirt's problem to solve, or you should expose the ability to set the global setting within QEMU. We can't just point our fingers at each other and hope the problem goes away :-) Hi Anthony, Currently, to resolve this problem, can we set 'pseries' as default? Because mac99 doesn't work at all on our platform. Thanks. :) -- Li Regards, Anthony Liguori Creating basic XML structure with relevant defaults pre-filled for a particular usecase is something that the libvirt-designer library is aiming to take care of for applications. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 3/4] qemu: Adapt qemuBuildInterfaceCommandLine to to multiqueue net
On 21.05.2013 18:37, Laine Stump wrote:
On 05/21/2013 10:18 AM, Michal Privoznik wrote:
In order to learn libvirt multiqueue several things must be done:
1) The '/dev/net/tun' device needs to be opened multiple times with
IFF_MULTI_QUEUE flag passed to ioctl(fd, TUNSETIFF, ifr);
2) Similar, the '/dev/vhost-net' must be opened as many times as in 1)
s/Similar, the/Similarly,/
in order to keep 1:1 ratio recommended by qemu and kernel folks.
3) The command line construction code needs to switch from 'fd=X' to
'fds=X:Y:...:Z' and from 'vhostfd=X' to 'vhostfds=X:Y:...:Z'.
4) The monitor handling code needs to learn to pass multiple FDs.
---
src/network/bridge_driver.c | 2 +-
src/qemu/qemu_command.c | 260
++--
src/qemu/qemu_command.h | 13 ++-
src/qemu/qemu_hotplug.c | 98 -
src/qemu/qemu_monitor.c | 78 +++--
src/qemu/qemu_monitor.h | 8 +-
src/uml/uml_conf.c | 5 +-
src/util/virnetdevtap.c | 113 ++-
src/util/virnetdevtap.h | 2 +
9 files changed, 378 insertions(+), 201 deletions(-)
ACK. (I'm undecided if we should error out if multi-queue is requested
when running non-privileged, and I don't care enough about the extra
braces to require you to add them (and it's not in the official coding
style))
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Okay, I squashed this in:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6011882..6203eec 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -371,7 +371,10 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
goto cleanup;
}
/* qemuCreateInBridgePortWithHelper can only create a single FD */
-*tapfdSize = 1;
+if (*tapfdSize 1) {
+VIR_WARN(Ignoring multiqueue network request);
+*tapfdSize = 1;
+}
}
virDomainAuditNetDevice(def, net, /dev/net/tun, true);
@@ -394,9 +397,9 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
}
if (net-filter net-ifname
-virDomainConfNWFilterInstantiate(conn, def-uuid, net) 0)
+virDomainConfNWFilterInstantiate(conn, def-uuid, net) 0) {
goto cleanup;
-
+}
ret = 0;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virNetDevTapCreate: Fail on systems not supporting IFF_MULTI_QUEUE
In my previous patches I enabled the IFF_MULTI_QUEUE flag every
time the user requested multiqueue TAP device. However, this
works only at runtime. During build time the flag may be
undeclared.
---
src/util/virnetdevtap.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index aa41b9c..e8057f7 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -178,8 +178,15 @@ int virNetDevTapCreate(char **ifname,
ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
/* If tapfdSize is greater than one, request multiqueue */
-if (tapfdSize 1)
+if (tapfdSize 1) {
+# ifdef IFF_MULTI_QUEUE
ifr.ifr_flags |= IFF_MULTI_QUEUE;
+# else
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(Multiqueue devices are not supported on this
system));
+goto cleanup;
+# endif
+}
# ifdef IFF_VNET_HDR
if ((flags VIR_NETDEV_TAP_CREATE_VNET_HDR)
--
1.8.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: add virGetGroupList
On 05/21/2013 11:24 PM, Eric Blake wrote:
Since neither getpwuid_r() nor initgroups() are safe to call in
between fork and exec (they obtain a mutex, but if some other
thread in the parent also held the mutex at the time of the fork,
the child will deadlock), we have to split out the functionality
that is unsafe. This patch adds a nice wrapper around
getgrouplist, which does the lookup by uid instead of name and
which mallocs the result; at least glibc's initgroups() uses
getgrouplist under the hood.
* configure.ac (AC_CHECK_FUNCS_ONCE): Check for getgrouplist.
* src/util/virutil.h (virGetGroupList): New declaration.
* src/util/virutil.c (virGetGroupList): New function.
* src/libvirt_private.syms (virutil.h): Export it.
Signed-off-by: Eric Blake ebl...@redhat.com
---
configure.ac | 4 +--
src/libvirt_private.syms | 1 +
src/util/virutil.c | 93
src/util/virutil.h | 2 ++
4 files changed, 98 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index 2055637..d20b90e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -205,8 +205,8 @@ AC_CHECK_SIZEOF([long])
dnl Availability of various common functions (non-fatal if missing),
dnl and various less common threadsafe functions
-AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \
- getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \
+AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getgrouplist \
+ getmntent_r getpwuid_r getuid initgroups kill mmap newlocale
posix_fallocate \
posix_memalign prlimit regexec sched_getaffinity setns setrlimit symlink])
dnl Availability of pthread functions (if missing, win32 threading is
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a81a865..3d9d89f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1933,6 +1933,7 @@ virGetDeviceID;
virGetDeviceUnprivSGIO;
virGetFCHostNameByWWN;
virGetGroupID;
+virGetGroupList;
virGetGroupName;
virGetHostname;
virGetUnprivSGIOSysfsPath;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 239fba8..a3cb64f 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -993,6 +993,99 @@ virGetGroupID(const char *group, gid_t *gid)
return 0;
}
+
+/* Compute the list of supplementary groups associated with @uid, and
+ * including @gid in the list (unless it is -1), storing a malloc'd
+ * result into @list. Return the size of the list on success, or -1
+ * on failure with error reported and errno set. May not be called
+ * between fork and exec. */
+int
+virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
+{
+char *buf = NULL;
+gid_t *groups = NULL;
+int ngroups = 0;
+int ret = -1;
+
+*list = NULL;
+if (uid == (uid_t)-1)
+return 0;
+
+# ifdef HAVE_GETGROUPLIST
+{
+struct passwd pwd, *pwd_result;
+size_t bufsize;
+int rc;
+
+bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
+if (bufsize == -1)
+bufsize = 16384;
+
+if (VIR_ALLOC_N(buf, bufsize) 0) {
+virReportOOMError();
+goto error;
+}
+while ((rc = getpwuid_r(uid, pwd, buf, bufsize,
+pwd_result)) == ERANGE) {
+if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) 0) {
+virReportOOMError();
+goto error;
+}
+}
+
+if (rc) {
+virReportSystemError(rc, _(cannot getpwuid_r(%u)),
+ (unsigned int) uid);
+goto error;
+}
+
+if (!pwd_result) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _(getpwuid_r failed to retrieve data
+ for uid '%u'),
+ (unsigned int) uid);
+goto error;
+}
Up to here, it's just direct movement from virSetUIDGID.
Now we get to the new stuff:
+
+if (gid == (gid_t)-1)
+gid = pwd.pw_gid;
The main use of the gid in the args is to specify one group that will
not be included in the returned group, and it's usually set to the group
listed in the pw record (got that from the manpage :-), so this makes sense.
+
+/* Figure out what size list to expect */
+getgrouplist(pwd.pw_name, gid, groups, ngroups);
Do we need to be concerned about the BUGS info in the manpage?
BUGS
In glibc versions before 2.3.3, the implementation of
this function contains a buffer-overrun bug: it returns
the complete list of groups for user in the array
groups, even when the number of groups exceeds *ngroups.
Is anyone running that vintage of glibc? It sounds *kind of* like it
could hit that if ngroups is 0 (but doesn't specifically say that).
+
+if
Re: [libvirt] [PATCH] virNetDevTapCreate: Fail on systems not supporting IFF_MULTI_QUEUE
On 05/22/2013 10:37 AM, Michal Privoznik wrote: In my previous patches I enabled the IFF_MULTI_QUEUE flag every time the user requested multiqueue TAP device. However, this works only at runtime. During build time the flag may be undeclared. --- src/util/virnetdevtap.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) ACK. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virNetDevTapCreate: Fail on systems not supporting IFF_MULTI_QUEUE
On 22.05.2013 18:50, Eric Blake wrote: On 05/22/2013 10:37 AM, Michal Privoznik wrote: In my previous patches I enabled the IFF_MULTI_QUEUE flag every time the user requested multiqueue TAP device. However, this works only at runtime. During build time the flag may be undeclared. --- src/util/virnetdevtap.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) ACK. Thanks, pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: add virGetGroupList
On 05/22/2013 10:49 AM, Laine Stump wrote:
On 05/21/2013 11:24 PM, Eric Blake wrote:
Since neither getpwuid_r() nor initgroups() are safe to call in
between fork and exec (they obtain a mutex, but if some other
thread in the parent also held the mutex at the time of the fork,
the child will deadlock), we have to split out the functionality
that is unsafe. This patch adds a nice wrapper around
getgrouplist, which does the lookup by uid instead of name and
which mallocs the result; at least glibc's initgroups() uses
getgrouplist under the hood.
* configure.ac (AC_CHECK_FUNCS_ONCE): Check for getgrouplist.
* src/util/virutil.h (virGetGroupList): New declaration.
* src/util/virutil.c (virGetGroupList): New function.
* src/libvirt_private.syms (virutil.h): Export it.
+
+/* Figure out what size list to expect */
+getgrouplist(pwd.pw_name, gid, groups, ngroups);
Do we need to be concerned about the BUGS info in the manpage?
BUGS
In glibc versions before 2.3.3, the implementation of
this function contains a buffer-overrun bug: it returns
the complete list of groups for user in the array
groups, even when the number of groups exceeds *ngroups.
Is anyone running that vintage of glibc? It sounds *kind of* like it
could hit that if ngroups is 0 (but doesn't specifically say that).
So I did some digging, and gnulib has an 'mgetgroups' module
(unfortunately GPL, but maybe I could get it relaxed if we wanted to use
that instead) that does the lookup by name rather than by uid_t. Here's
what gnulib's code says about the issue:
#if HAVE_GETGROUPLIST
/* We prefer to use getgrouplist if available, because it has better
performance characteristics.
In glibc 2.3.2, getgrouplist is buggy. If you pass a zero as the
length of the output buffer, getgrouplist will still write to the
buffer. Contrary to what some versions of the getgrouplist
manpage say, this doesn't happen with nonzero buffer sizes.
Therefore our usage here just avoids a zero sized buffer. */
if (username)
{
enum { N_GROUPS_INIT = 10 };
max_n_groups = N_GROUPS_INIT;
g = realloc_groupbuf (NULL, max_n_groups);
So it looks like I could indeed work around the bug by providing a
non-zero buffer in my probing version, and even go so far as to
pre-allocate a reasonable size buffer to get by with a single
getgrouplist() call if the given uid is in a small number of groups to
begin with (and what do you know - our most common victim uid will be
qemu, which on a default-install Fedora system is uid 107 and a member
of exactly two groups: kvm(36) and qemu(107)).
Version 2 coming up with that idea incorporated.
+
+if (!ngroups gid != (gid_t)-1) {
+if (VIR_ALLOC_N(groups, 1) 0) {
+virReportOOMError();
+goto error;
+}
+groups[ngroups++] = gid;
A reasonable fallback.
The gnulib module actually goes one further, and crawls through
getgrent() looking for any group that mentions uid (that is, instead of
ignoring supplementary groups, it actually tries a slower method of
getting at them). Maybe I'll pursue getting the gnulib module relaxed
to LGPL after all.
ACK. I'm curious about whether the bug listed in the manpage could ever
hit us, though (surely it can't be *that* bad, or if it is then everyone
will have a patch for it).
I'll do a v2 that either works around the bug, or which delegates to the
gnulib module (depending on response on the gnulib list about a license
relax request).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 03/13] Change virConnectDomainEventGraphicsCallback signature
On 21.05.2013 01:20, Eric Blake wrote: On 05/20/2013 11:55 AM, Michal Privoznik wrote: For future work we need _virDomainEventGraphicsAddress and _virDomainEventGraphicsSubjectIdentity members to be char * not const char *. We are strdup()ing them anyway, so they should have been char * anyway (from const correctness POV). However, we don't want users to change passed values, so we need to make the callback's argument const. --- include/libvirt/libvirt.h.in | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) Please update your commit message to include a bit more justification - this part of my analysis on v3 (slightly reworded here) would be a good start: Although this is an API change, real callers won't be impacted. Why? 1. these callback members are read-only, so it is less likely that someone is trying to assign into the struct members. 2. The only way to register a virConnectDomainEventGraphicsCallback is to cast it through a call to virConnectDomainEventRegisterAny. That is, even if the user's callback function leaves out the const, we never use the typedef as the direct type of any API parameter. Since they are already casting their function pointer into a munged type before registering it, their code will continue to compile. ACK with that improvement. Okay, I've pushed the first three patches. Thanks so far. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH RESENT 04/12] libxl: populate xenstore memory entries at startup\
On Wed, 22 May 2013, Jim Fehlig wrote:
Marek Marczykowski wrote:
On 19.04.2013 13:10, Stefano Stabellini wrote:
On Thu, 11 Apr 2013, Marek Marczykowski wrote:
On 11.04.2013 09:52, Ian Campbell wrote:
On Thu, 2013-04-11 at 05:09 +0100, Jim Fehlig wrote:
+/* This will fill xenstore info about free and dom0 memory - if
missing,
+ * should be called before starting first domain */
+if (libxl_get_free_memory(libxl_driver-ctx, free_mem)) {
+VIR_ERROR(_(cannot get free memory info));
+goto error;
+}
Should failure of libxl_get_free_memory() really be fatal and prevent
the driver from loading?
I'm not sure it is intended to be called like this...
I think it is intended to be called as part of starting every domain, to
check if there is enough free memory for that domain, rather than
calling it once at start of day.
In that context if it fails or returns less than the required amount of
memory then that would be fatal for starting that domain.
In xl we use this as part of the auto balloon of dom0, see
xl_cmdimplg.c:freemem. Does libvirt do autoballooning or does it require
dom0_mem? Perhaps this is handled at a higher level?
The problem is how libxl set initial value for freemem-slack. If, for any
reason, dom0 hasn't (almost) all memory assigned before creating first
domain,
15% of host memory will no longer be used at all. This any reason can be
dom0_mem, which is covered by auto value for autoballoon in xen-unstable
(actually only for xl, not libxl in general).
This is the (in)famous incompatibility between autoballoon and dom0_mem.
But this can also happen if
somebody calls xl set-mem 0 some value. The later case doesn't mean the
user
want to disable autoballoon completely.
Calling xl set-mem 0 manually should be compatible with
autoballoon.
Unless it is called before first domain start (which case is covered by my
patch).
However it wouldn't work with dom0_mem.
And to answer you question - libvirt rely on libxl autoballoon.
Could we introduce something similar to autoballoon=auto to libvirt?
Maybe we should push down the autoballoon option to libxl: we should
probably rename autoballoon to libxl_memory_management, enable it
automatically during initialization (and call
libxl__fill_dom0_memory_info) unless dom0_mem has been passed as an
option, in which case we would disable it. If we do it at the libxl
level we would cover xl as well as libvirt and also fix the xl set-mem
0 case.
Looks good for me, but IIUC it's too late for such change in 4.3 and it
doesn't qualify for later backport, right? If so, some approach still is
needed in libvirt for xen 4.2 and 4.3 (like my simple patch). This still
doesn't make libvirt compatible with dom0_mem, but at least cover one
(independent) case. Perhaps additionally autoballoon=auto code should be
copied from xl to libvirt to cover dom0_mem case with xen 4.2 and 4.3?
After reading this thread again, it sounds like the best option is to
support the autoballoon option in libvirt, e.g. in
/etc/libvirt/libxl.conf. (We currently don't have a config file for the
libxl driver, but I think we'll need one anyhow for other knobs, similar
to qemu.conf.) As you note, even if autoballoon is pushed down to
libxl, libvirt would need to handle it for older libxl. And libvirt
needs to handle the dom0_mem case...
Given all the troubles that we had with the autoballoon option in
xl/libxl and how it clashes with dom0_mem, I would strongly advise to
consider implementing something like autoballoon=auto from the start.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] util: make virSetUIDGID async-signal-safe
On 05/21/2013 11:24 PM, Eric Blake wrote: https://bugzilla.redhat.com/show_bug.cgi?id=964358 POSIX states that multi-threaded apps should not use functions that are not async-signal-safe between fork and exec, yet we were using getpwuid_r and initgroups. Although rare, it is possible to hit deadlock in the child, when it tries to grab a mutex that was already held by another thread in the parent. I actually hit this deadlock when testing multiple domains being started in parallel with a command hook, with the following backtrace in the child: Thread 1 (Thread 0x7fd56bbf2700 (LWP 3212)): #0 __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:136 #1 0x7fd5761e7388 in _L_lock_854 () from /lib64/libpthread.so.0 #2 0x7fd5761e7257 in __pthread_mutex_lock (mutex=0x7fd56be00360) at pthread_mutex_lock.c:61 #3 0x7fd56bbf9fc5 in _nss_files_getpwuid_r (uid=0, result=0x7fd56bbf0c70, buffer=0x7fd55c2a65f0 , buflen=1024, errnop=0x7fd56bbf25b8) at nss_files/files-pwd.c:40 #4 0x7fd575aeff1d in __getpwuid_r (uid=0, resbuf=0x7fd56bbf0c70, buffer=0x7fd55c2a65f0 , buflen=1024, result=0x7fd56bbf0cb0) at ../nss/getXXbyYY_r.c:253 #5 0x7fd578aebafc in virSetUIDGID (uid=0, gid=0) at util/virutil.c:1031 #6 0x7fd578aebf43 in virSetUIDGIDWithCaps (uid=0, gid=0, capBits=0, clearExistingCaps=true) at util/virutil.c:1388 #7 0x7fd578a9a20b in virExec (cmd=0x7fd55c231f10) at util/vircommand.c:654 #8 0x7fd578a9dfa2 in virCommandRunAsync (cmd=0x7fd55c231f10, pid=0x0) at util/vircommand.c:2247 #9 0x7fd578a9d74e in virCommandRun (cmd=0x7fd55c231f10, exitstatus=0x0) at util/vircommand.c:2100 #10 0x7fd56326fde5 in qemuProcessStart (conn=0x7fd53c000df0, driver=0x7fd55c0dc4f0, vm=0x7fd54800b100, migrateFrom=0x0, stdin_fd=-1, stdin_path=0x0, snapshot=0x0, vmop=VIR_NETDEV_VPORT_PROFILE_OP_CREATE, flags=1) at qemu/qemu_process.c:3694 ... The solution is to split the work of getpwuid_r/initgroups into the unsafe portions (getgrouplist, called pre-fork) and safe portions (setgroups, called post-fork). * src/util/virutil.h (virSetUIDGID, virSetUIDGIDWithCaps): Adjust signature. * src/util/virutil.c (virSetUIDGID): Add parameters. (virSetUIDGIDWithCaps): Adjust clients. * src/util/vircommand.c (virExec): Likewise. * src/util/virfile.c (virFileAccessibleAs, virFileOpenForked) (virDirCreate): Likewise. * src/security/security_dac.c (virSecurityDACSetProcessLabel): Likewise. * configure.ac (AC_CHECK_FUNCS_ONCE): Check for setgroups, not initgroups. Signed-off-by: Eric Blake ebl...@redhat.com --- configure.ac| 5 +- src/security/security_dac.c | 16 +-- src/util/vircommand.c | 10 +++- src/util/virfile.c | 30 ++-- src/util/virutil.c | 108 src/util/virutil.h | 5 +- 6 files changed, 84 insertions(+), 90 deletions(-) diff --git a/configure.ac b/configure.ac index d20b90e..26deac7 100644 --- a/configure.ac +++ b/configure.ac @@ -206,8 +206,9 @@ AC_CHECK_SIZEOF([long]) dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getgrouplist \ - getmntent_r getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \ - posix_memalign prlimit regexec sched_getaffinity setns setrlimit symlink]) + getmntent_r getpwuid_r getuid kill mmap newlocale posix_fallocate \ + posix_memalign prlimit regexec sched_getaffinity setgroups setns \ + setrlimit symlink]) dnl Availability of pthread functions (if missing, win32 threading is dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. diff --git a/src/security/security_dac.c b/src/security/security_dac.c index d922ad2..907dbd5 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1000,17 +1000,25 @@ virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr, uid_t user; gid_t group; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); +gid_t *groups; +int ngroups; +int ret = -1; if (virSecurityDACGetIds(def, priv, user, group)) return -1; +ngroups = virGetGroupList(user, group, groups); +if (ngroups 0) +return -1; VIR_DEBUG(Dropping privileges of DEF to %u:%u, (unsigned int) user, (unsigned int) group); -if (virSetUIDGID(user, group) 0) -return -1; - -return 0; +if (virSetUIDGID(user, group, groups, ngroups) 0) +goto cleanup; +ret = 0; +cleanup: +VIR_FREE(groups); +return ret; } diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 665f549..ba03802 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -402,6
[libvirt] Schedule for next release ?
It seems that if we want to have the release at the end of the month we ought to enter freeze next Monday the 27 for a release on the 3rd (I may have a hard time making the release on Friday 31 or w.e.). Is there any problem with freezing next Monday ? If not let's proceed that way, in that case 1.0.6 will be out on Monday 3rd June ! Daniel -- Daniel Veillard | Open Source and Standards, Red Hat veill...@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 05/13] Adapt to VIR_STRDUP and VIR_STRNDUP in src/qemu/*
On 05/20/2013 11:55 AM, Michal Privoznik wrote:
---
src/qemu/qemu_capabilities.c | 79 +++-
src/qemu/qemu_cgroup.c | 4 +-
src/qemu/qemu_command.c | 428
+--
src/qemu/qemu_conf.c | 64 +++
src/qemu/qemu_domain.c | 26 +--
src/qemu/qemu_driver.c | 129 -
src/qemu/qemu_hotplug.c | 15 +-
src/qemu/qemu_migration.c| 17 +-
src/qemu/qemu_monitor_json.c | 63 ++-
src/qemu/qemu_monitor_text.c | 15 +-
src/qemu/qemu_process.c | 64 +++
11 files changed, 333 insertions(+), 571 deletions(-)
@@ -1912,12 +1899,11 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr
qemuCaps,
if (VIR_ALLOC(mach) 0)
goto no_memory;
if (qemuCaps-machineAliases[i]) {
-if (!(mach-name = strdup(qemuCaps-machineAliases[i])))
-goto no_memory;
-if (!(mach-canonical = strdup(qemuCaps-machineTypes[i])))
+if (VIR_STRDUP(mach-name, qemuCaps-machineAliases[i]) 0 ||
+VIR_STRDUP(mach-canonical, qemuCaps-machineTypes[i]) 0)
goto no_memory;
silent-noisy - probably a good change, but may be worth
s/no_memory/error/ in the label so I don't assume it's a double-oom.
+++ b/src/qemu/qemu_command.c
@@ -8549,17 +8524,16 @@ static int qemuStringToArgvEnv(const char *args,
next = strchr(curr, '\n');
if (next) {
-arg = strndup(curr, next-curr);
+if (VIR_STRNDUP(arg, curr, next - curr) 0)
+goto error;
if (*next == '\'' ||
*next == '')
next++;
} else {
-arg = strdup(curr);
+if (VIR_STRDUP(arg, curr) 0)
+goto error;
}
Perhaps worth shortening to:
if (VIR_STRNDUP(arg, curr, next ? next - curr : strlen(curr)) 0)
goto error;
but I'll leave that as your call.
@@ -9416,40 +9372,32 @@ qemuParseCommandLineChr(virDomainChrSourceDefPtr
source,
host2 = svc1 ? strchr(svc1, '@') : NULL;
svc2 = host2 ? strchr(host2, ':') : NULL;
-if (svc1 (svc1 != val)) {
-source-data.udp.connectHost = strndup(val, svc1-val);
-
-if (!source-data.udp.connectHost)
-goto no_memory;
-}
+if (svc1 svc1 != val
+VIR_STRNDUP(source-data.udp.connectHost, val, svc1 - val) 0)
+goto error;
if (svc1) {
svc1++;
-if (host2)
-source-data.udp.connectService = strndup(svc1, host2-svc1);
-else
-source-data.udp.connectService = strdup(svc1);
-if (!source-data.udp.connectService)
-goto no_memory;
+if ((host2 VIR_STRNDUP(source-data.udp.connectService,
+ svc1, host2 - svc1) 0) ||
+(!host2 VIR_STRDUP(source-data.udp.connectService,
+ svc1) 0))
+goto error;
Looks simpler as:
if (VIR_STRNDUP(source-dta.udp.connectService, sv1,
host2 ? host2 - svc1 : strlen(svc1)) 0)
goto error;
Hmm, this makes me wonder if it is worth making VIR_STRNDUP slightly
smarter, where if the length argument is -1, then it uses strlen
internally, so we could get away with clients written as:
if (VIR_STRNDUP(source-dta.udp.connectService, sv1,
host2 ? host2 - svc1 : -1) 0)
After all, we've done magic like that for virBufferAdd (pass -1 instead
of strlen, and virBuffer does the right thing on your behalf). But if
we DO decide to add the magic, it should be in a standalone patch and
with a testsuite addition.
@@ -9472,37 +9420,25 @@ qemuParseCommandLineChr(virDomainChrSourceDefPtr
source,
if (opt strstr(opt, server))
source-data.tcp.listen = true;
-source-data.tcp.host = strndup(val, svc-val);
-if (!source-data.tcp.host)
-goto no_memory;
+if (VIR_STRNDUP(source-data.tcp.host, val, svc - val) 0)
+goto error;
svc++;
-if (opt) {
-source-data.tcp.service = strndup(svc, opt-svc);
-} else {
-source-data.tcp.service = strdup(svc);
-}
-if (!source-data.tcp.service)
-goto no_memory;
+if ((opt VIR_STRNDUP(source-data.tcp.service, svc, opt - svc)
0) ||
+(!opt VIR_STRDUP(source-data.tcp.service, svc) 0))
Again, looks nicer as:
if (VIR_STRNDUP(source-data.tcp.service, svc,
opt ? opt - svc : strlen(svc)) 0)
[okay, so maybe the magic -1 really is nicer than making clients write
strlen]
+goto error;
} else if (STRPREFIX(val, unix:)) {
const char *opt;
val += strlen(unix:);
opt = strchr(val, ',');
source-type =
Re: [libvirt] [PATCH v4 06/13] Adapt to VIR_STRDUP and VIR_STRNDUP in src/remote/*
On 05/20/2013 11:55 AM, Michal Privoznik wrote:
---
src/remote/remote_driver.c | 114
+
1 file changed, 44 insertions(+), 70 deletions(-)
@@ -497,24 +497,21 @@ doRemoteOpen(virConnectPtr conn,
...
if (conn-uri conn-uri-user
-!(username = strdup(conn-uri-user)))
-goto no_memory;
+VIR_STRDUP(username, conn-uri-user) 0)
+goto failed;
Could be simplified to:
if (conn-uri VIR_STRDUP(username, conn-uri-user) 0)
@@ -705,17 +696,13 @@ doRemoteOpen(virConnectPtr conn,
break;
case trans_ssh:
-if (!command !(command = strdup(ssh)))
-goto no_memory;
+if (!command VIR_STRDUP(command, ssh) 0)
+goto failed;
-if (!sockname) {
-if (flags VIR_DRV_OPEN_REMOTE_RO)
-sockname = strdup(LIBVIRTD_PRIV_UNIX_SOCKET_RO);
-else
-sockname = strdup(LIBVIRTD_PRIV_UNIX_SOCKET);
-if (!sockname)
-goto no_memory;
-}
+if (VIR_STRDUP(sockname,
Memory leak if sockname was already defined. Must be:
if (!sockname VIR_STRDUP(sockname, ...
@@ -4472,15 +4449,15 @@ remoteDomainBuildEventGraphics(virNetClientProgramPtr
prog ATTRIBUTE_UNUSED,
if (VIR_ALLOC(localAddr) 0)
goto no_memory;
localAddr-family = msg-local.family;
-if (!(localAddr-service = strdup(msg-local.service)) ||
-!(localAddr-node = strdup(msg-local.node)))
+if (VIR_STRDUP(localAddr-service, msg-local.service) 0 ||
+VIR_STRDUP(localAddr-node, msg-local.node) 0)
goto no_memory;
silent-noisy, when we are really just discarding the event on any
error. Based on discussion earlier in the series, that means we are
polluting the thread-local error object with no one to report it, but
that the pollution will be cleaned up on the next real API that we
handle. So I think I can live with the change to noisy. However,
s/no_memory/error/ would be appropriate.
ACK with those fixes.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Remove OPTION section in output of 'virsh help command' if no option exists.
On 05/21/2013 09:15 PM, Zhang Xiaohe wrote:
Don't print 'OPTION' if there's no options. Just behaves as DESCRIPTION
does.
This mostly affects 'interface' command group.
Signed-off-by: Zhang Xiaohe zhan...@cn.fujitsu.com
Reported-by: Li Yang liyang.f...@cn.fujitsu.com
---
tools/virsh.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
For some reason, the patch didn't apply for me with 'git am', so I had
to do it by hand; in the process, I simplified slightly.
diff --git a/tools/virsh.c b/tools/virsh.c
index ecb7bd4..7c60800 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1270,7 +1270,9 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname)
if (def-opts) {
const vshCmdOptDef *opt;
-fputs(_(\n OPTIONS\n), stdout);
+/* Print the option only if there are options */
+if (def-opts-name)
+fputs(_(\n OPTIONS\n), stdout);
Hmm, I wonder why we even bother to create 1-element arrays with a NULL
terminator instead of passing NULL when registering option-less
functions, on commands like 'iface-commit'. But your idea is fine. ACK
and here's what I pushed, after tweaking the subject line to be shorter:
diff --git a/tools/virsh.c b/tools/virsh.c
index ecb7bd4..6f0c1ef 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1268,7 +1268,7 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname)
fprintf(stdout, %s\n, _(desc));
}
-if (def-opts) {
+if (def-opts def-opts-name) {
const vshCmdOptDef *opt;
fputs(_(\n OPTIONS\n), stdout);
for (opt = def-opts; opt-name; opt++) {
--
1.8.1.4
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] need custom /dev entries in LXC
Hi, We run nvidia devices inside libvirt-managed LXC containers. It used to be that simply doing: $ echo 'c 195:* rwm' /sys/fs/cgroup/devices/libvirt/lxc Then, after booting the container, we would do: $ mknod -m 666 /dev/nvidia0 c 195 0 would be good enough to run our CUDA applications. But, according to: $ cat src/lxc/lxc_container.c The CAP_MKNOD capability is being dropped and only a specific set of devices is being created before booting the container. Is there any reason why this is not per-device configurable? Thanks, - Michael R. Hines -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 07/13] Adapt to VIR_STRDUP and VIR_STRNDUP in src/rpc/*
On 05/20/2013 11:55 AM, Michal Privoznik wrote: --- src/rpc/gendispatch.pl | 21 src/rpc/virnetclient.c | 16 - src/rpc/virnetmessage.c | 27 +-- src/rpc/virnetsaslcontext.c | 6 ++-- src/rpc/virnetserver.c | 6 ++-- src/rpc/virnetserverclient.c | 10 ++ src/rpc/virnetservermdns.c | 6 ++-- src/rpc/virnetsocket.c | 10 +++--- src/rpc/virnetsshsession.c | 78 +--- src/rpc/virnettlscontext.c | 26 +++ 10 files changed, 92 insertions(+), 114 deletions(-) ACK. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] two hostdev devices problem
I tested on 1.0.5 patched version and vm with 2 vfs working fine. I have another problem When i set max_vfs=63: internal error missing IFLA_VF_INFO in netlink response Its working when max_vfs=31 -- Dominik 21 maj 2013 15:19, Dominik Mostowiec dominikmostow...@gmail.com napisał(a): Hmm, It seems to be working (after only simple tests). -- Dominik 2013/5/21 Ján Tomko jto...@redhat.com On 05/21/2013 01:37 PM, Laine Stump wrote: On 05/21/2013 04:03 AM, Ján Tomko wrote: On 05/21/2013 09:32 AM, Dominik Mostowiec wrote: hi, I try to add 2 VF by hostdev. Networks (vnet0, vnet1) with: forward mode='hostdev' managed='yes' pf dev='eth1'/ . Domain: interface type=network source network=vnet0/ interface type=network source network=vnet1/ virsh create error: error: internal error process exited while connecting to monitor: kvm: -device pci-assign,configfd=25,host=01:10.1,id=hostdev0,bus=pci.0,addr=0x4: Duplicate ID 'hostdev0' for device Hi, it seems we have been assigning the same id to all network hostdevs until this recent commit (not yet released): http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6597cc25 If that patch has such an effect, it's purely accidental. Have you tested this? (I plan to test a before and after as soon as I've had breakfast) I haven't tested it and looking at the code again, I might've been wrong :( Sorry about that. Jan -- Pozdrawiam Dominik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 08/13] Adapt to VIR_STRDUP and VIR_STRNDUP in src/security/*
On 05/20/2013 11:55 AM, Michal Privoznik wrote: --- src/security/security_apparmor.c | 20 ++ src/security/security_dac.c | 21 +++ src/security/security_nop.c | 7 +--- src/security/security_selinux.c | 79 +++- src/security/virt-aa-helper.c| 4 +- 5 files changed, 35 insertions(+), 96 deletions(-) ACK. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Ongoing work on lock contention in qemu driver?
One theory I had was that the virDomainObjListSearchName method could be a bottleneck, becaue that acquires a lock on every single VM. This is invoked when starting a VM, when we call virDomainObjListAddLocked. I tried removing this locking though didn't see any performance benefit, so never persued this further. Before trying things like this again, I think we'd need to find a way to actually identify where the true bottlenecks are, rather than guesswork. ... Oh someone has already written such a systemtap script http://sourceware.org/systemtap/examples/process/mutex-contention.stp I think that is preferrable to trying to embed special code in libvirt for this task. 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 :| Cool! The systemtap approach was very fruitful. BTW, at the time of writing, the example script has a bug. See http://sourceware.org/ml/systemtap/2013-q2/msg00169.html for the fix. So the root cause of my bottleneck is the virSecurityManager lock. From this root cause a few other bottlenecks emerge. The interesting parts of the mutex-contention.stp report are pasted at the end of this email. Here's the summary my analysis: When a domain is created (domainCreateWithFlags), the domain object's lock is held. During the domain creation, various virSecurity functions are called, which all grab the security manager's lock. Since the security manager's lock is global, some fraction of domainCreateWithFlags is serialized by this lock. Since some virSecurity functions can take a long time, such as virSecurityManagerGenLabel for the apparmor security driver, which takes around 1s, the serialization that the security manager lock induces in domainCreateWithFlags is substantial. Since the domain's object lock is held all of this time, virDomainObjListSearchName blocks, thereby serializing virConnectDefineXML via virDomainObjListAdd, as you suggested earlier. Moreover, since the virDomainObjList lock is held while blocking in virDomainObjListSearchName, there's measurable contention whilst looking up domains during domainCreateWithFlags. Since some security driver operations are costly, I think it's worthwhile to reduce the scope of the security manager lock or increase the granularity by introducing more locks. After a cursory look, the security manager lock seems to have a much broader scope than necessary. The system / library calls underlying the security drivers are all thread safe (e.g., defining apparmor security profiles or chowning disk files), so a global lock isn't strictly necessary. Moreover, since most virSecurity calls are made whilst a virDomainObj lock is held and the security calls are generally domain specific, *most* of the security calls are probably thread safe in the absence of the global security manager lock. Obviously some work will have to be done to see where the security lock actually matters and some finer-grained locks will have to be introduced to handle these situations. I also think it's worthwhile to eliminate locking from the the virDomainObjList lookups and traversals. Since virDomainObjLists are accessed in a bunch of places, I think it's a good defensive idea to decouple the performance of these accesses from virDomainObj locks, which are held during potentially long-running operations like domain creation. An easy way to divorce virDomainObjListSearchName from the virDomainObj lock would be to keep a copy of the domain names in the virDomainObjList and protect that list with the virDomainObjList lock. What do you think? Peter == stack contended 4 times, 261325 avg usec, 576521 max usec, 1045301 total usec, at __lll_lock_wait+0x1c [libpthread-2.15.so] _L_lock_858+0xf [libpthread-2.15.so] __pthread_mutex_lock+0x3a [libpthread-2.15.so] virDomainObjListFindByUUID+0x21 [libvirt.so.0.1000.4] qemuDomainGetXMLDesc+0x48 [libvirt_driver_qemu.so] virDomainGetXMLDesc+0xf5 [libvirt.so.0.1000.4] remoteDispatchDomainGetXMLDescHelper+0xb6 [libvirtd] virNetServerProgramDispatch+0x498 [libvirt.so.0.1000.4] virNetServerProcessMsg+0x2a [libvirt.so.0.1000.4] virNetServerHandleJob+0x73 [libvirt.so.0.1000.4] virThreadPoolWorker+0x10e == stack contended 12 times, 128053 avg usec, 992567 max usec, 1536640 total usec, at __lll_lock_wait+0x1c [libpthread-2.15.so] _L_lock_858+0xf [libpthread-2.15.so] __pthread_mutex_lock+0x3a [libpthread-2.15.so] virDomainObjListFindByUUID+0x21 [libvirt.so.0.1000.4] qemuDomainStartWithFlags+0x5a [libvirt_driver_qemu.so] virDomainCreateWithFlags+0xf5 [libvirt.so.0.1000.4] remoteDispatchDomainCreateWithFlagsHelper+0xbe [libvirtd] virNetServerProgramDispatch+0x498 [libvirt.so.0.1000.4] virNetServerProcessMsg+0x2a
Re: [libvirt] [PATCH RESENT 03/12] libxl: nodeDevice* support for PCI devices
On 22.05.2013 16:36, Jim Fehlig wrote:
Marek Marczykowski wrote:
For now only for PCI devices. Mostly copy-paste from old xen driver.
This one is (or will be) covered by Chanyan's work as well right?
Right.
Regards,
Jim
---
src/libxl/libxl_driver.c | 193
+++
1 file changed, 193 insertions(+)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 40a7a6b..011edf8 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -42,6 +42,7 @@
#include libxl.h
#include libxl_driver.h
#include libxl_conf.h
+#include node_device_conf.h
#include xen_xm.h
#include virtypedparam.h
#include viruri.h
@@ -3666,6 +3667,195 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const
char *xml,
return libxlDomainModifyDeviceFlags(dom, xml, flags,
LIBXL_DEVICE_UPDATE);
}
+static int
+libxlNodeDeviceGetPciInfo(virNodeDevicePtr dev,
+ unsigned *domain,
+ unsigned *bus,
+ unsigned *slot,
+ unsigned *function)
+{
+virNodeDeviceDefPtr def = NULL;
+virNodeDevCapsDefPtr cap;
+char *xml = NULL;
+int ret = -1;
+
+xml = virNodeDeviceGetXMLDesc(dev, 0);
+if (!xml)
+goto out;
+
+def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
+if (!def)
+goto out;
+
+cap = def-caps;
+while (cap) {
+if (cap-type == VIR_NODE_DEV_CAP_PCI_DEV) {
+*domain = cap-data.pci_dev.domain;
+*bus = cap-data.pci_dev.bus;
+*slot = cap-data.pci_dev.slot;
+*function = cap-data.pci_dev.function;
+break;
+}
+
+cap = cap-next;
+}
+
+if (!cap) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _(device %s is not a PCI device), dev-name);
+goto out;
+}
+
+ret = 0;
+out:
+virNodeDeviceDefFree(def);
+VIR_FREE(xml);
+return ret;
+}
+
+static int
+libxlNodeDeviceDettach(virNodeDevicePtr dev)
+{
+virPCIDevicePtr pci;
+unsigned domain, bus, slot, function;
+int ret = -1;
+
+if (libxlNodeDeviceGetPciInfo(dev, domain, bus, slot, function) 0)
+return -1;
+
+pci = virPCIDeviceNew(domain, bus, slot, function);
+if (!pci)
+return -1;
+
+if (virPCIDeviceDetach(pci, NULL, NULL, pciback) 0)
+goto out;
+
+ret = 0;
+out:
+virPCIDeviceFree(pci);
+return ret;
+}
+
+static int
+libxlNodeDeviceAssignedDomainId(virNodeDevicePtr dev)
+{
+int numdomains;
+int numpcidevs;
+int ret = -1, i, j;
+int *ids = NULL;
+char *bdf = NULL;
+char *xref = NULL;
+unsigned int domain, bus, slot, function;
+libxl_device_pci *pcidevs = NULL;
+virConnectPtr conn = dev-conn;
+libxlDriverPrivatePtr driver = conn-privateData;
+
+/* Get active domains */
+numdomains = libxlNumDomains(conn);
+if (numdomains 0) {
+return ret;
+}
+if (numdomains 0){
+if (VIR_ALLOC_N(ids, numdomains) 0) {
+virReportOOMError();
+goto out;
+}
+if ((numdomains = libxlListDomains(conn, ids[0], numdomains)) 0)
{
+goto out;
+}
+}
+
+/* Get pci bdf */
+if (libxlNodeDeviceGetPciInfo(dev, domain, bus, slot, function) 0)
+goto out;
+
+if (virAsprintf(bdf, %04x:%02x:%02x.%0x,
+domain, bus, slot, function) 0) {
+virReportOOMError();
+goto out;
+}
+
+libxlDriverLock(driver);
+/* Check if bdf is assigned to one of active domains */
+for (i = 0; i numdomains; i++) {
+pcidevs = libxl_device_pci_list(driver-ctx, ids[i], numpcidevs);
+if (pcidevs == NULL) {
+continue;
+} else {
+for (j = 0; j numpcidevs; j++) {
+if (pcidevs[j].bus == bus pcidevs[j].dev == slot
pcidevs[j].func == function) {
+ret = ids[i];
+break;
+}
+}
+VIR_FREE(pcidevs);
+}
+}
+libxlDriverUnlock(driver);
+
+VIR_FREE(xref);
+VIR_FREE(bdf);
+out:
+VIR_FREE(ids);
+
+return ret;
+}
+
+static int
+libxlNodeDeviceReAttach(virNodeDevicePtr dev)
+{
+virPCIDevicePtr pci;
+unsigned domain, bus, slot, function;
+int ret = -1;
+int domid;
+
+if (libxlNodeDeviceGetPciInfo(dev, domain, bus, slot, function) 0)
+return -1;
+
+pci = virPCIDeviceNew(domain, bus, slot, function);
+if (!pci)
+return -1;
+
+/* Check if device is assigned to an active guest */
+if ((domid = libxlNodeDeviceAssignedDomainId(dev)) = 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+
Re: [libvirt] [PATCH RESENT 02/12] libxl: PCI passthrough support
On 22.05.2013 16:33, Jim Fehlig wrote: Jim Fehlig wrote: Daniel P. Berrange wrote: On Wed, Apr 10, 2013 at 04:44:43AM +0200, Marek Marczykowski wrote: --- src/libxl/libxl_conf.c | 72 ++ src/libxl/libxl_conf.h | 2 ++ 2 files changed, 74 insertions(+) This needs todo more than just create the config. You need to use the libvirt PCI APIs to validate that it is safe to assign the devices requested, and track which devices are given to which guest, to avoid concurrent assignemnt to 2 guests. Basically I'd expect this to borrow all the code currently in the QMEU driver related to the 'activePciHostdevs' and 'inactivePciHostdevs' data structures. Right. Chunyan already posted such a patch series [1], but Laine noted that it would be useful to maintain the state of PCI device assignment for coordination among multiple drivers. Chunyan posted a design [2] for a hostdev passthrough driver, and IIRC she has started coding after receiving feedback from the list. I see that Chunyan has now posted patches for this work https://www.redhat.com/archives/libvir-list/2013-May/msg01162.html Could you can help review and test those? Sure, will look at it tomorrow. -- Best Regards, Marek Marczykowski Invisible Things Lab signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH RESENT 04/12] libxl: populate xenstore memory entries at startup\
On 22.05.2013 18:58, Stefano Stabellini wrote:
On Wed, 22 May 2013, Jim Fehlig wrote:
Marek Marczykowski wrote:
On 19.04.2013 13:10, Stefano Stabellini wrote:
On Thu, 11 Apr 2013, Marek Marczykowski wrote:
On 11.04.2013 09:52, Ian Campbell wrote:
On Thu, 2013-04-11 at 05:09 +0100, Jim Fehlig wrote:
+/* This will fill xenstore info about free and dom0 memory - if
missing,
+ * should be called before starting first domain */
+if (libxl_get_free_memory(libxl_driver-ctx, free_mem)) {
+VIR_ERROR(_(cannot get free memory info));
+goto error;
+}
Should failure of libxl_get_free_memory() really be fatal and prevent
the driver from loading?
I'm not sure it is intended to be called like this...
I think it is intended to be called as part of starting every domain, to
check if there is enough free memory for that domain, rather than
calling it once at start of day.
In that context if it fails or returns less than the required amount of
memory then that would be fatal for starting that domain.
In xl we use this as part of the auto balloon of dom0, see
xl_cmdimplg.c:freemem. Does libvirt do autoballooning or does it require
dom0_mem? Perhaps this is handled at a higher level?
The problem is how libxl set initial value for freemem-slack. If, for any
reason, dom0 hasn't (almost) all memory assigned before creating first
domain,
15% of host memory will no longer be used at all. This any reason can be
dom0_mem, which is covered by auto value for autoballoon in xen-unstable
(actually only for xl, not libxl in general).
This is the (in)famous incompatibility between autoballoon and dom0_mem.
But this can also happen if
somebody calls xl set-mem 0 some value. The later case doesn't mean the
user
want to disable autoballoon completely.
Calling xl set-mem 0 manually should be compatible with
autoballoon.
Unless it is called before first domain start (which case is covered by my
patch).
However it wouldn't work with dom0_mem.
And to answer you question - libvirt rely on libxl autoballoon.
Could we introduce something similar to autoballoon=auto to libvirt?
Maybe we should push down the autoballoon option to libxl: we should
probably rename autoballoon to libxl_memory_management, enable it
automatically during initialization (and call
libxl__fill_dom0_memory_info) unless dom0_mem has been passed as an
option, in which case we would disable it. If we do it at the libxl
level we would cover xl as well as libvirt and also fix the xl set-mem
0 case.
Looks good for me, but IIUC it's too late for such change in 4.3 and it
doesn't qualify for later backport, right? If so, some approach still is
needed in libvirt for xen 4.2 and 4.3 (like my simple patch). This still
doesn't make libvirt compatible with dom0_mem, but at least cover one
(independent) case. Perhaps additionally autoballoon=auto code should be
copied from xl to libvirt to cover dom0_mem case with xen 4.2 and 4.3?
After reading this thread again, it sounds like the best option is to
support the autoballoon option in libvirt, e.g. in
/etc/libvirt/libxl.conf. (We currently don't have a config file for the
libxl driver, but I think we'll need one anyhow for other knobs, similar
to qemu.conf.) As you note, even if autoballoon is pushed down to
libxl, libvirt would need to handle it for older libxl. And libvirt
needs to handle the dom0_mem case...
Given all the troubles that we had with the autoballoon option in
xl/libxl and how it clashes with dom0_mem, I would strongly advise to
consider implementing something like autoballoon=auto from the start.
Ok, will send such patch (almost ready). For now it will contain copypaste
auto_autobaloon() from xl.c, so the same logic. Can be easily extended to
config option in the future, but probably I will not have time for this.
--
Best Regards,
Marek Marczykowski
Invisible Things Lab
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] remote: fix dom-id after virDomainCreateWithFlags
The same issue as (already fixed) in virDomainCreate -
REMOTE_PROC_DOMAIN_CREATE_WITH_FLAGS doesn't return new domain ID, only
-1 on error or 0 on success.
Besides this one fix it is more general problem - local domain object
ID can desynchronize with the real one, for example in case of another
client creates/destroys domain in the meantime. Perhaps virDomainGetID
should be called remotely (with all performance implications...)? Or
some event-based notification used?
Signed-off-by: Marek Marczykowski marma...@invisiblethingslab.com
---
src/remote/remote_driver.c | 40
src/remote/remote_protocol.x | 2 +-
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index f66304c..0bbfc22 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -2423,6 +2423,46 @@ done:
return rv;
}
+static int
+remoteDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
+{
+int rv = -1;
+struct private_data *priv = dom-conn-privateData;
+remote_domain_create_with_flags_args args;
+remote_domain_lookup_by_uuid_args args2;
+remote_domain_lookup_by_uuid_ret ret2;
+
+remoteDriverLock(priv);
+
+make_nonnull_domain(args.dom, dom);
+args.flags = flags;
+
+if (call(dom-conn, priv, 0, REMOTE_PROC_DOMAIN_CREATE_WITH_FLAGS,
+ (xdrproc_t)xdr_remote_domain_create_with_flags_args, (char
*)args,
+ (xdrproc_t)xdr_void, (char *)NULL) == -1) {
+goto done;
+}
+
+/* Need to do a lookup figure out ID of newly started guest, because
+ * bug in design of REMOTE_PROC_DOMAIN_CREATE_WITH_FLAGS means we aren't
getting
+ * it returned.
+ */
+memcpy(args2.uuid, dom-uuid, VIR_UUID_BUFLEN);
+memset(ret2, 0, sizeof(ret2));
+if (call(dom-conn, priv, 0, REMOTE_PROC_DOMAIN_LOOKUP_BY_UUID,
+ (xdrproc_t) xdr_remote_domain_lookup_by_uuid_args, (char *)
args2,
+ (xdrproc_t) xdr_remote_domain_lookup_by_uuid_ret, (char *) ret2)
== -1)
+goto done;
+
+dom-id = ret2.dom.id;
+xdr_free((xdrproc_t) xdr_remote_domain_lookup_by_uuid_ret, (char *)
ret2);
+rv = 0;
+
+done:
+remoteDriverUnlock(priv);
+return rv;
+}
+
static char *
remoteDomainGetSchedulerType(virDomainPtr domain, int *nparams)
{
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 512ba2e..d2e0175 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -3875,7 +3875,7 @@ enum remote_procedure {
REMOTE_PROC_DOMAIN_EVENT_IO_ERROR_REASON = 195,
/**
- * @generate: both
+ * @generate: server
*/
REMOTE_PROC_DOMAIN_CREATE_WITH_FLAGS = 196,
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Remove OPTION section in output of 'virsh help command' if no option exists.
于 2013年05月23日 06:19, Eric Blake 写道:
On 05/21/2013 09:15 PM, Zhang Xiaohe wrote:
Don't print 'OPTION' if there's no options. Just behaves as DESCRIPTION
does.
This mostly affects 'interface' command group.
Signed-off-by: Zhang Xiaohezhan...@cn.fujitsu.com
Reported-by: Li Yangliyang.f...@cn.fujitsu.com
---
tools/virsh.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
For some reason, the patch didn't apply for me with 'git am', so I had
to do it by hand; in the process, I simplified slightly.
diff --git a/tools/virsh.c b/tools/virsh.c
index ecb7bd4..7c60800 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1270,7 +1270,9 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname)
if (def-opts) {
const vshCmdOptDef *opt;
-fputs(_(\n OPTIONS\n), stdout);
+/* Print the option only if there are options */
+if (def-opts-name)
+fputs(_(\n OPTIONS\n), stdout);
Hmm, I wonder why we even bother to create 1-element arrays with a NULL
terminator instead of passing NULL when registering option-less
functions, on commands like 'iface-commit'. But your idea is fine. ACK
and here's what I pushed, after tweaking the subject line to be shorter:
diff --git a/tools/virsh.c b/tools/virsh.c
index ecb7bd4..6f0c1ef 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1268,7 +1268,7 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname)
fprintf(stdout, %s\n, _(desc));
}
-if (def-opts) {
+if (def-opts def-opts-name) {
const vshCmdOptDef *opt;
fputs(_(\n OPTIONS\n), stdout);
for (opt = def-opts; opt-name; opt++) {
Thanks, this looks more pretty~
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 09/12] LXC: controller: change the owner of /dev to the root user of container
container will create /dev/pts directory in /dev.
the owner of /dev should be the root user of container.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_controller.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 4660f25..f892ce3 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1191,6 +1191,13 @@ virLXCControllerSetupDev(virLXCControllerPtr ctrl)
char *opts = NULL;
char *dev = NULL;
int ret = -1;
+uid_t uid = (uid_t)-1;
+gid_t gid = (gid_t)-1;
+
+if (ctrl-def-idmap.uidmap) {
+uid = ctrl-def-idmap.uidmap[0].target;
+gid = ctrl-def-idmap.gidmap[0].target;
+}
VIR_DEBUG(Setting up /dev/ for container);
@@ -1231,6 +1238,13 @@ virLXCControllerSetupDev(virLXCControllerPtr ctrl)
goto cleanup;
}
+if (chown(dev, uid, gid) 0) {
+virReportSystemError(errno,
+ _(Failed to change owner of %s to %d:%d),
+ dev, uid, gid);
+goto cleanup;
+}
+
ret = 0;
cleanup:
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 07/12] LXC: fuse: Change files owner to the root user of container
Otherwise we will fail to mount the meminfo file.
This patch also allows any users to access the
fuse mount point.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_fuse.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c
index b6df99c..ddb737c 100644
--- a/src/lxc/lxc_fuse.c
+++ b/src/lxc/lxc_fuse.c
@@ -49,6 +49,9 @@ static int lxcProcGetattr(const char *path, struct stat
*stbuf)
char *mempath = NULL;
struct stat sb;
+struct fuse_context *context = fuse_get_context();
+virDomainDefPtr def = (virDomainDefPtr)context-private_data;
+
memset(stbuf, 0, sizeof(struct stat));
if (virAsprintf(mempath, /proc/%s, path) 0) {
virReportOOMError();
@@ -66,6 +69,8 @@ static int lxcProcGetattr(const char *path, struct stat
*stbuf)
goto cleanup;
}
+stbuf-st_uid = def-idmap.uidmap ? def-idmap.uidmap[0].target : 0;
+stbuf-st_gid = def-idmap.gidmap ? def-idmap.gidmap[0].target : 0;
stbuf-st_mode = sb.st_mode;
stbuf-st_nlink = 1;
stbuf-st_blksize = sb.st_blksize;
@@ -307,6 +312,7 @@ int lxcSetupFuse(virLXCFusePtr *f, virDomainDefPtr def)
/* process name is libvirt_lxc */
if (fuse_opt_add_arg(args, libvirt_lxc) == -1 ||
fuse_opt_add_arg(args, -odirect_io) == -1 ||
+fuse_opt_add_arg(args, -oallow_other) == -1 ||
fuse_opt_add_arg(args, -ofsname=libvirt) == -1)
goto cleanup1;
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 03/12] LXC: sort the uidmap/gidmap of domain
Make sure the mapping line contains the root user of container
is the first element of idmap array. So we can get the real
user id on host for the container easily.
This patch also check the map information, User must map
the root user of container to any user of host.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/conf/domain_conf.c | 20
1 file changed, 20 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a3c5c84..e271f5a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10061,6 +10061,14 @@ cleanup:
}
+static int virDomainIdMapEntrySort(const void *a, const void *b)
+{
+const virDomainIdMapEntryPtr entrya = (const virDomainIdMapEntryPtr) a;
+const virDomainIdMapEntryPtr entryb = (const virDomainIdMapEntryPtr) b;
+
+return entrya-start entryb-start;
+}
+
/* Parse the XML definition for user namespace id map.
*
* idmap has the form of
@@ -10088,6 +10096,18 @@ virDomainIdmapDefParseXML(const xmlNodePtr *node,
virXPathUInt(string(./@target), ctxt, idmap[i].target);
virXPathUInt(string(./@count), ctxt, idmap[i].count);
}
+
+qsort(idmap, num, sizeof(idmap[0]), virDomainIdMapEntrySort);
+
+if (idmap[0].start != 0) {
+/* Root user of container hasn't been mapped to any user of host,
+ * return error. */
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(You must map the root user of container));
+VIR_FREE(idmap);
+idmap = NULL;
+}
+
error:
ctxt-node = save_ctxt;
return idmap;
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 08/12] LXC: controller: change the owner of tty devices to the root user of container
Since these tty devices will be used by container,
the owner of them should be the root user of container.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_controller.c | 43 +--
1 file changed, 37 insertions(+), 6 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 7d10660..4660f25 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1380,13 +1380,14 @@ static int lxcSetPersonality(virDomainDefPtr def)
* *TTYNAME. Heavily borrowed from glibc, but doesn't require that
* devpts == /dev/pts */
static int
-lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName)
+lxcCreateTty(virLXCControllerPtr ctrl, int *ttymaster,
+ char **ttyName, char **ttyHostPath)
{
int ret = -1;
int ptyno;
int unlock = 0;
-if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) 0)
+if ((*ttymaster = open(ctrl-devptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) 0)
goto cleanup;
if (ioctl(*ttymaster, TIOCSPTLCK, unlock) 0)
@@ -1407,6 +1408,13 @@ lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName)
goto cleanup;
}
+if (virAsprintf(ttyHostPath, /%s/%s.devpts/%d, LXC_STATE_DIR,
+ctrl-def-name, ptyno) 0) {
+virReportOOMError();
+errno = ENOMEM;
+goto cleanup;
+}
+
ret = 0;
cleanup:
@@ -1552,18 +1560,41 @@ virLXCControllerSetupConsoles(virLXCControllerPtr ctrl,
char **containerTTYPaths)
{
size_t i;
+int ret = -1;
+uid_t uid = (uid_t)-1;
+gid_t gid = (gid_t)-1;
+char *ttyHostPath = NULL;
+
+if (ctrl-def-idmap.uidmap) {
+uid = ctrl-def-idmap.uidmap[0].target;
+gid = ctrl-def-idmap.gidmap[0].target;
+}
for (i = 0; i ctrl-nconsoles; i++) {
VIR_DEBUG(Opening tty on private %s, ctrl-devptmx);
-if (lxcCreateTty(ctrl-devptmx,
+if (lxcCreateTty(ctrl,
ctrl-consoles[i].contFd,
- containerTTYPaths[i]) 0) {
+ containerTTYPaths[i], ttyHostPath) 0) {
virReportSystemError(errno, %s,
_(Failed to allocate tty));
-return -1;
+goto out;
}
+
+/* Change the owner of tty device to the root user of container */
+if (chown(ttyHostPath, uid, gid) 0) {
+virReportSystemError(errno,
+ _(Failed to change owner of tty
+ %s to %u:%u),
+ ttyHostPath, uid, gid);
+goto out;
+}
+VIR_FREE(ttyHostPath);
}
-return 0;
+
+ret = 0;
+out:
+VIR_FREE(ttyHostPath);
+return ret;
}
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 11/12] LXC: controller: change the owner of /dev/pts and ptmx to the root of container
This two files are created for container,
the owner should be the root user of container.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_controller.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index b2ace20..7d27135 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1506,8 +1506,15 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
char *opts = NULL;
char *devpts = NULL;
char *path = NULL;
+uid_t uid = (uid_t)-1;
+gid_t gid = (gid_t)-1;
int ret = -1;
+if (ctrl-def-idmap.uidmap) {
+uid = ctrl-def-idmap.uidmap[0].target;
+gid = ctrl-def-idmap.gidmap[0].target;
+}
+
VIR_DEBUG(Setting up private /dev/pts);
mount_options = virSecurityManagerGetMountOptions(ctrl-securityManager,
@@ -1551,6 +1558,21 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
goto cleanup;
}
+if (chown(ctrl-devptmx, uid, gid) 0) {
+virReportSystemError(errno,
+ _(Failed to change the owner of
+%s to %u:%u),
+ path, uid, gid);
+goto cleanup;
+}
+if (chown(devpts, uid, gid) 0) {
+virReportSystemError(errno,
+ _(Failed to change the owner of
+ %s to %u:%u),
+ devpts, uid, gid);
+goto cleanup;
+}
+
if (access(ctrl-devptmx, W_OK) 0) {
if (virAsprintf(path, /%s/%s.dev/ptmx,
LXC_STATE_DIR, ctrl-def-name)) {
@@ -1564,8 +1586,16 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
virReportSystemError(errno, _(Failed to make device %s), path);
goto cleanup;
}
+if (chown(path, uid, gid) 0) {
+virReportSystemError(errno,
+ _(Failed to change the owner of
+ %s to %u:%u),
+ path, uid, gid);
+goto cleanup;
+}
}
+
ret = 0;
cleanup:
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 02/12] LXC: enable user namespace only when user set the uidmap
User namespace will be enabled only when the idmap exist
in configuration.
If you want disable user namespace,just remove these
elements from XML.
If kernel doesn't support user namespace and idmap exist
in configuration file, libvirt lxc will start failed and
return Kernel doesn't support user namespace message.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_container.c | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index c74e3ca..618252c 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -2029,14 +2029,12 @@ cleanup:
static int userns_supported(void)
{
-#if 1
-/*
- * put off using userns until uid mapping is implemented
- */
-return 0;
-#else
return lxcContainerAvailable(LXC_CONTAINER_FEATURE_USER) == 0;
-#endif
+}
+
+static int userns_required(virDomainDefPtr def)
+{
+ return def-idmap.uidmap def-idmap.gidmap;
}
virArch lxcContainerGetAlt32bitArch(virArch arch)
@@ -2116,9 +2114,15 @@ int lxcContainerStart(virDomainDefPtr def,
cflags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|SIGCHLD;
-if (userns_supported()) {
-VIR_DEBUG(Enable user namespaces);
-cflags |= CLONE_NEWUSER;
+if (userns_required(def)) {
+if (userns_supported()) {
+VIR_DEBUG(Enable user namespace);
+cflags |= CLONE_NEWUSER;
+} else {
+virReportSystemError(VIR_ERR_NO_KERNEL, %s,
+ _(Kernel doesn't support user namespace));
+return -1;
+}
}
if (lxcNeedNetworkNamespace(def)) {
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 00/12] Add user namespace support for libvirt lxc
This patchset try to add userns support for libvirt lxc. Since userns is nearly completed in linux-3.9, the old kernel doesn't support userns, I add some New XML elements to let people decide if enable userns.The userns is enabled only when user configure the XML. The format of user namespace related XML file like below: idmap uid start='0' target='1000' count='10' gid start='0' target='1000' count='10' /idmap it means the user in container (which uid:gid is 0:0) will be mapped to the user in host (uid:gid is 1000:1000), count is used to form an u/gid range: The users in container which uid in [start, start + count -1] will be mapped. You can have multiple lines to map differnet id ranges, caution, you must make sure the root user of container has been mapped. This patchset also does the below jobs. 1, Because the uninit userns has no right to create devices, we should create devices for container on host. 2, Changes the owner of fuse and tty device. Change from v2: 1, Mount tmpfs on /stateDir/domain.dev 2, Create devices under /stateDir/doamin.dev/ 3, Mount Move the /.oldroot/stateDir/doamin.dev/ on the /dev/ of container 4, Enhance the configuration, disallow the semi configuration Gao feng (12): LXC: Introduce New XML element for user namespace LXC: enable user namespace only when user set the uidmap LXC: sort the uidmap/gidmap of domain LXC: introduce virLXCControllerSetupUserns and lxcContainerSetID LXC: Creating devices for container on host side LXC: Move creating /dev/ptmx to virLXCControllerSetupDevPTS LXC: fuse: Change files owner to the root user of container LXC: controller: change the owner of tty devices to the root user of container LXC: controller: change the owner of /dev to the root user of container LXC: controller: change the owner of devices created on host LXC: controller: change the owner of /dev/pts and ptmx to the root of container LXC: introduce virLXCControllerChown docs/formatdomain.html.in | 23 docs/schemas/domaincommon.rng | 31 + src/conf/domain_conf.c| 115 ++ src/conf/domain_conf.h| 22 src/lxc/lxc_container.c | 183 ++-- src/lxc/lxc_controller.c | 271 +- src/lxc/lxc_fuse.c| 6 + 7 files changed, 557 insertions(+), 94 deletions(-) -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 06/12] LXC: Move creating /dev/ptmx to virLXCControllerSetupDevPTS
Make codes clearer and reduce some virAsprintf.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_controller.c | 44 +++-
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index e9808f3..7d10660 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1245,7 +1245,6 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
{
size_t i;
int ret = -1;
-char *ptmx = NULL;
char *path = NULL;
const struct {
int maj;
@@ -1283,30 +1282,8 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
VIR_FREE(path);
}
-if (virAsprintf(ptmx, /%s/%s.devpts/ptmx,
-LXC_STATE_DIR, ctrl-def-name) 0) {
-virReportOOMError();
-goto out;
-}
-
-if (access(ptmx, W_OK)) {
-if (virAsprintf(path, /%s/%s.dev/ptmx,
-LXC_STATE_DIR, ctrl-def-name)) {
-virReportOOMError();
-goto out;
-}
-/* Legacy devpts, so we need to just use shared one */
-dev_t dev = makedev(LXC_DEV_MAJ_TTY, LXC_DEV_MIN_PTMX);
-if (mknod(path, S_IFCHR, dev) 0 ||
-chmod(path, 0666)) {
-virReportSystemError(errno, _(Failed to make device %s), path);
-goto out;
-}
-}
-
ret = 0;
out:
-VIR_FREE(ptmx);
VIR_FREE(path);
return ret;
}
@@ -1492,6 +1469,7 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
char *mount_options = NULL;
char *opts = NULL;
char *devpts = NULL;
+char *path = NULL;
int ret = -1;
VIR_DEBUG(Setting up private /dev/pts);
@@ -1537,9 +1515,25 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
goto cleanup;
}
+if (access(ctrl-devptmx, W_OK) 0) {
+if (virAsprintf(path, /%s/%s.dev/ptmx,
+LXC_STATE_DIR, ctrl-def-name)) {
+virReportOOMError();
+goto cleanup;
+}
+/* Legacy devpts, so we need to just use shared one */
+dev_t dev = makedev(LXC_DEV_MAJ_TTY, LXC_DEV_MIN_PTMX);
+if (mknod(path, S_IFCHR, dev) 0 ||
+chmod(path, 0666)) {
+virReportSystemError(errno, _(Failed to make device %s), path);
+goto cleanup;
+}
+}
+
ret = 0;
cleanup:
+VIR_FREE(path);
VIR_FREE(opts);
VIR_FREE(devpts);
VIR_FREE(mount_options);
@@ -1708,10 +1702,10 @@ virLXCControllerRun(virLXCControllerPtr ctrl)
if (virLXCControllerSetupResourceLimits(ctrl, cgroup) 0)
goto cleanup;
-if (virLXCControllerSetupDevPTS(ctrl) 0)
+if (virLXCControllerPopulateDevices(ctrl) 0)
goto cleanup;
-if (virLXCControllerPopulateDevices(ctrl) 0)
+if (virLXCControllerSetupDevPTS(ctrl) 0)
goto cleanup;
if (virLXCControllerSetupFuse(ctrl) 0)
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 05/12] LXC: Creating devices for container on host side
user namespace doesn't allow to create devices in
uninit userns. We should create devices on host side.
We first mount tmpfs on dev directroy under state dir
of container. then create devices under this dev dir.
Finally in container, mount the dev directroy created
on host to the /dev/ directroy of container.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_container.c | 96 +-
src/lxc/lxc_controller.c | 130 +++
2 files changed, 166 insertions(+), 60 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 52fcf39..c647f8e 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -683,7 +683,7 @@ err:
}
-static int lxcContainerMountBasicFS(char *sec_mount_options)
+static int lxcContainerMountBasicFS(void)
{
const struct {
const char *src;
@@ -711,8 +711,6 @@ static int lxcContainerMountBasicFS(char *sec_mount_options)
int i, rc = -1;
char *opts = NULL;
-VIR_DEBUG(Mounting basic filesystems sec_mount_options=%s,
sec_mount_options);
-
for (i = 0; i ARRAY_CARDINALITY(mnts); i++) {
const char *srcpath = NULL;
@@ -750,26 +748,6 @@ static int lxcContainerMountBasicFS(char
*sec_mount_options)
}
}
-/*
- * tmpfs is limited to 64kb, since we only have device nodes in there
- * and don't want to DOS the entire OS RAM usage
- */
-
-if (virAsprintf(opts,
-mode=755,size=65536%s, sec_mount_options) 0) {
-virReportOOMError();
-goto cleanup;
-}
-
-VIR_DEBUG(Mount devfs on /dev type=tmpfs flags=%x, opts=%s,
- MS_NOSUID, opts);
-if (mount(devfs, /dev, tmpfs, MS_NOSUID, opts) 0) {
-virReportSystemError(errno,
- _(Failed to mount %s on %s type %s (%s)),
- devfs, /dev, tmpfs, opts);
-goto cleanup;
-}
-
rc = 0;
cleanup:
@@ -811,6 +789,33 @@ static int lxcContainerMountProcFuse(virDomainDefPtr def
ATTRIBUTE_UNUSED,
}
#endif
+static int lxcContainerMountFSDev(virDomainDefPtr def,
+ const char *stateDir)
+{
+int ret;
+char *path = NULL;
+
+VIR_DEBUG(Mount /dev/ stateDir=%s, stateDir);
+
+if ((ret = virAsprintf(path,
+ /.oldroot/%s/%s.dev,
+ stateDir,
+ def-name)) 0)
+return ret;
+
+VIR_DEBUG(Trying to move %s to /dev, path);
+
+if ((ret = mount(path, /dev,
+ NULL, MS_MOVE, NULL)) 0) {
+virReportSystemError(errno,
+ _(Failed to mount %s on /dev),
+ path);
+}
+
+VIR_FREE(path);
+return ret;
+}
+
static int lxcContainerMountFSDevPTS(virDomainDefPtr def,
const char *stateDir)
{
@@ -847,22 +852,10 @@ cleanup:
return ret;
}
-static int lxcContainerPopulateDevices(char **ttyPaths, size_t nttyPaths)
+static int lxcContainerSetupDevices(char **ttyPaths, size_t nttyPaths)
{
size_t i;
const struct {
-int maj;
-int min;
-mode_t mode;
-const char *path;
-} devs[] = {
-{ LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_NULL, 0666, /dev/null },
-{ LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_ZERO, 0666, /dev/zero },
-{ LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_FULL, 0666, /dev/full },
-{ LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_RANDOM, 0666, /dev/random },
-{ LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_URANDOM, 0666, /dev/urandom },
-};
-const struct {
const char *src;
const char *dst;
} links[] = {
@@ -872,18 +865,6 @@ static int lxcContainerPopulateDevices(char **ttyPaths,
size_t nttyPaths)
{ /proc/self/fd, /dev/fd },
};
-/* Populate /dev/ with a few important bits */
-for (i = 0; i ARRAY_CARDINALITY(devs); i++) {
-dev_t dev = makedev(devs[i].maj, devs[i].min);
-if (mknod(devs[i].path, S_IFCHR, dev) 0 ||
-chmod(devs[i].path, devs[i].mode)) {
-virReportSystemError(errno,
- _(Failed to make device %s),
- devs[i].path);
-return -1;
-}
-}
-
for (i = 0; i ARRAY_CARDINALITY(links); i++) {
if (symlink(links[i].src, links[i].dst) 0) {
virReportSystemError(errno,
@@ -903,15 +884,6 @@ static int lxcContainerPopulateDevices(char **ttyPaths,
size_t nttyPaths)
_(Failed to bind /dev/pts/ptmx on to
/dev/ptmx));
return -1;
}
-} else {
-/* Legacy devpts, so we need to just use shared one */
-dev_t dev = makedev(LXC_DEV_MAJ_TTY, LXC_DEV_MIN_PTMX);
-if (mknod(/dev/ptmx, S_IFCHR, dev) 0 ||
-chmod(/dev/ptmx, 0666)) {
-virReportSystemError(errno, %s,
-
[libvirt] [PATCH v3 04/12] LXC: introduce virLXCControllerSetupUserns and lxcContainerSetID
This patch introduces new helper function
virLXCControllerSetupUserns, in this function,
we set the files uid_map and gid_map of the init
task of container.
lxcContainerSetID is used for creating cred for
tasks running in container. Since after setuid/setgid,
we may be a new user. This patch calls lxcContainerSetUserns
at first to make sure the new created files belong to
right user.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_container.c | 63 --
src/lxc/lxc_controller.c | 65
2 files changed, 110 insertions(+), 18 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 618252c..52fcf39 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -335,6 +335,30 @@ int lxcContainerWaitForContinue(int control)
/**
+ * lxcContainerSetID:
+ *
+ * This function calls setuid and setgid to create proper
+ * cred for tasks running in container.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcContainerSetID(virDomainDefPtr def)
+{
+/* Only call virSetUIDGID when user namespace is enabled
+ * for this container. And user namespace is only enabled
+ * when nuidmapngidmap is not zero */
+
+if (def-idmap.nuidmap virSetUIDGID(0, 0) 0) {
+virReportSystemError(errno, %s,
+ _(setuid or setgid failed));
+return -1;
+}
+
+return 0;
+}
+
+
+/**
* lxcContainerRenameAndEnableInterfaces:
* @nveths: number of interfaces
* @veths: interface names
@@ -1937,6 +1961,27 @@ static int lxcContainerChild(void *data)
cmd = lxcContainerBuildInitCmd(vmDef);
virCommandWriteArgLog(cmd, 1);
+if (lxcContainerResolveSymlinks(vmDef) 0)
+goto cleanup;
+
+if (!virFileExists(vmDef-os.init)) {
+virReportSystemError(errno,
+_(cannot find init path '%s' relative to container root),
+vmDef-os.init);
+goto cleanup;
+}
+
+/* Wait for interface devices to show up */
+if (lxcContainerWaitForContinue(argv-monitor) 0) {
+virReportSystemError(errno, %s,
+ _(Failed to read the container continue
message));
+goto cleanup;
+}
+VIR_DEBUG(Received container continue message);
+
+if (lxcContainerSetID(vmDef) 0)
+goto cleanup;
+
root = virDomainGetRootFilesystem(vmDef);
if (argv-nttyPaths) {
@@ -1962,29 +2007,11 @@ static int lxcContainerChild(void *data)
goto cleanup;
}
-if (lxcContainerResolveSymlinks(vmDef) 0)
-goto cleanup;
-
if (lxcContainerSetupPivotRoot(vmDef, root,
argv-ttyPaths, argv-nttyPaths,
argv-securityDriver) 0)
goto cleanup;
-if (!virFileExists(vmDef-os.init)) {
-virReportSystemError(errno,
-_(cannot find init path '%s' relative to container root),
-vmDef-os.init);
-goto cleanup;
-}
-
-/* Wait for interface devices to show up */
-if (lxcContainerWaitForContinue(argv-monitor) 0) {
-virReportSystemError(errno, %s,
- _(Failed to read the container continue
message));
-goto cleanup;
-}
-VIR_DEBUG(Received container continue message);
-
/* rename and enable interfaces */
if (lxcContainerRenameAndEnableInterfaces(!!(vmDef-features
(1
VIR_DOMAIN_FEATURE_PRIVNET)),
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index dfe686b..0a2e3ac 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1122,6 +1122,68 @@ cleanup2:
}
+static int
+virLXCControllerSetupUsernsMap(virDomainIdMapEntryPtr map,
+ char *path,
+ int num)
+{
+virBuffer map_value = VIR_BUFFER_INITIALIZER;
+int i, ret = -1;
+
+for (i = 0; i num; i++)
+virBufferAsprintf(map_value, %u %u %u\n,
+ map[i].start, map[i].target, map[i].count);
+
+if (virFileWriteStr(path, virBufferCurrentContent(map_value), 0) 0) {
+virReportSystemError(errno, _(unable write to %s), path);
+goto cleanup;
+}
+ret = 0;
+
+cleanup:
+virBufferFreeAndReset(map_value);
+return ret;
+}
+
+/**
+ * virLXCControllerSetupUserns
+ *
+ * Set proc files for user namespace
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int virLXCControllerSetupUserns(virLXCControllerPtr ctrl)
+{
+char *uid_map = NULL;
+char *gid_map = NULL;
+int ret = -1;
+
+/* User namespace is disabled for container */
+if (ctrl-def-idmap.nuidmap == 0)
+return 0;
+
+if (virAsprintf(uid_map, /proc/%d/uid_map, ctrl-initpid) 0)
+goto cleanup;
+
+if (virLXCControllerSetupUsernsMap(ctrl-def-idmap.uidmap,
[libvirt] [PATCH v3 12/12] LXC: introduce virLXCControllerChown
use virLXCControllerChown to make codes clearer.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_controller.c | 81 ++--
1 file changed, 31 insertions(+), 50 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 7d27135..1cceecf 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1121,6 +1121,29 @@ cleanup2:
return rc;
}
+static int
+virLXCControllerChown(virLXCControllerPtr ctrl,
+ char *path)
+{
+uid_t uid = -1;
+gid_t gid = -1;
+
+if (!ctrl-def-idmap.uidmap)
+return 0;
+
+uid = ctrl-def-idmap.uidmap[0].target;
+gid = ctrl-def-idmap.gidmap[0].target;
+
+if (chown(path, uid, gid) 0) {
+virReportSystemError(errno,
+ _(Failed to change owner of %s to %u:%u),
+ path, uid, gid);
+return -1;
+}
+
+return 0;
+}
+
static int
virLXCControllerSetupUsernsMap(virDomainIdMapEntryPtr map,
@@ -1260,8 +1283,6 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;
char *path = NULL;
-uid_t uid = (uid_t)-1;
-gid_t gid = (gid_t)-1;
const struct {
int maj;
int min;
@@ -1278,11 +1299,6 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
if (virLXCControllerSetupDev(ctrl) 0)
goto out;
-if (ctrl-def-idmap.uidmap) {
-uid = ctrl-def-idmap.uidmap[0].target;
-gid = ctrl-def-idmap.gidmap[0].target;
-}
-
/* Populate /dev/ with a few important bits */
for (i = 0 ; i ARRAY_CARDINALITY(devs) ; i++) {
if (virAsprintf(path, /%s/%s.dev/%s,
@@ -1301,12 +1317,9 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
goto out;
}
-if (chown(path, uid, gid) 0) {
-virReportSystemError(errno,
- _(Failed to change owner of %s to %u:%u),
- devs[i].path, uid, gid);
+if (virLXCControllerChown(ctrl, path) 0)
goto out;
-}
+
VIR_FREE(path);
}
@@ -1506,15 +1519,8 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
char *opts = NULL;
char *devpts = NULL;
char *path = NULL;
-uid_t uid = (uid_t)-1;
-gid_t gid = (gid_t)-1;
int ret = -1;
-if (ctrl-def-idmap.uidmap) {
-uid = ctrl-def-idmap.uidmap[0].target;
-gid = ctrl-def-idmap.gidmap[0].target;
-}
-
VIR_DEBUG(Setting up private /dev/pts);
mount_options = virSecurityManagerGetMountOptions(ctrl-securityManager,
@@ -1558,20 +1564,11 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
goto cleanup;
}
-if (chown(ctrl-devptmx, uid, gid) 0) {
-virReportSystemError(errno,
- _(Failed to change the owner of
-%s to %u:%u),
- path, uid, gid);
+if (virLXCControllerChown(ctrl, ctrl-devptmx) 0)
goto cleanup;
-}
-if (chown(devpts, uid, gid) 0) {
-virReportSystemError(errno,
- _(Failed to change the owner of
- %s to %u:%u),
- devpts, uid, gid);
+
+if (virLXCControllerChown(ctrl, devpts) 0)
goto cleanup;
-}
if (access(ctrl-devptmx, W_OK) 0) {
if (virAsprintf(path, /%s/%s.dev/ptmx,
@@ -1586,13 +1583,8 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
virReportSystemError(errno, _(Failed to make device %s), path);
goto cleanup;
}
-if (chown(path, uid, gid) 0) {
-virReportSystemError(errno,
- _(Failed to change the owner of
- %s to %u:%u),
- path, uid, gid);
+if (virLXCControllerChown(ctrl, path) 0)
goto cleanup;
-}
}
@@ -1619,15 +1611,8 @@ virLXCControllerSetupConsoles(virLXCControllerPtr ctrl,
{
size_t i;
int ret = -1;
-uid_t uid = (uid_t)-1;
-gid_t gid = (gid_t)-1;
char *ttyHostPath = NULL;
-if (ctrl-def-idmap.uidmap) {
-uid = ctrl-def-idmap.uidmap[0].target;
-gid = ctrl-def-idmap.gidmap[0].target;
-}
-
for (i = 0; i ctrl-nconsoles; i++) {
VIR_DEBUG(Opening tty on private %s, ctrl-devptmx);
if (lxcCreateTty(ctrl,
@@ -1639,13 +1624,9 @@ virLXCControllerSetupConsoles(virLXCControllerPtr ctrl,
}
/* Change the owner of tty device to the root user of container */
-if (chown(ttyHostPath, uid, gid) 0) {
-virReportSystemError(errno,
- _(Failed to change owner of tty
- %s to %u:%u),
- ttyHostPath, uid, gid);
[libvirt] [PATCH v3 10/12] LXC: controller: change the owner of devices created on host
Since these devices are created for the container.
the owner should be the root user of the container.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_controller.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index f892ce3..b2ace20 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1260,6 +1260,8 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;
char *path = NULL;
+uid_t uid = (uid_t)-1;
+gid_t gid = (gid_t)-1;
const struct {
int maj;
int min;
@@ -1276,6 +1278,11 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
if (virLXCControllerSetupDev(ctrl) 0)
goto out;
+if (ctrl-def-idmap.uidmap) {
+uid = ctrl-def-idmap.uidmap[0].target;
+gid = ctrl-def-idmap.gidmap[0].target;
+}
+
/* Populate /dev/ with a few important bits */
for (i = 0 ; i ARRAY_CARDINALITY(devs) ; i++) {
if (virAsprintf(path, /%s/%s.dev/%s,
@@ -1293,6 +1300,13 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
devs[i].path);
goto out;
}
+
+if (chown(path, uid, gid) 0) {
+virReportSystemError(errno,
+ _(Failed to change owner of %s to %u:%u),
+ devs[i].path, uid, gid);
+goto out;
+}
VIR_FREE(path);
}
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 01/12] LXC: Introduce New XML element for user namespace
This patch introduces new element idmap for
user namespace. for example
idmap
uid start='0' target='1000' count='10'/
gid start='0' target='1000' count='10'/
/idmap
this new element is used for setting proc files
/proc/pid/{uid_map,gid_map}.
This patch also supports multiple uid/gid elements
setting in XML configuration.
We don't support the semi configuation, user has to
configure uid and gid both.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
docs/formatdomain.html.in | 23 +++
docs/schemas/domaincommon.rng | 31 ++
src/conf/domain_conf.c| 95 +++
src/conf/domain_conf.h| 22 ++
4 files changed, 171 insertions(+)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 3a200aa..cc0ae52 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -285,6 +285,29 @@
/pre
+p
+ If you want to enable user namespace,set the codeidmap/code element.
+ the codeuid/code and codegid/code elements have three attributes:
+/p
+
+dl
+ dtcodestart/code/dt
+ ddFirst user id in container./dd
+ dtcodetarget/code/dt
+ ddThe first user id in container will be mapped to this target user
+ id in host./dd
+ dtcodecount/code/dt
+ ddHow many users in container being allowed to map to host's user./dd
+/dl
+
+pre
+ lt;idmapgt;
+lt;uid start='0' target='1000' count='10'/gt;
+lt;gid start='0' target='1000' count='10'/gt;
+ lt;/idmapgt;
+/pre
+
+
h3a name=elementsSysinfoSMBIOS System Information/a/h3
p
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3cace35..ac07407 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -56,6 +56,9 @@
ref name=pm/
/optional
optional
+ ref name=idmap/
+/optional
+optional
ref name=devices/
/optional
zeroOrMore
@@ -465,6 +468,34 @@
/optional
/interleave
/define
+ define name=idmap
+zeroOrMore
+ element name=uid
+attribute name=start
+ ref name=unsignedInt/
+/attribute
+attribute name=target
+ ref name=unsignedInt/
+/attribute
+attribute name=count
+ ref name=unsignedInt/
+/attribute
+ /element
+/zeroOrMore
+zeroOrMore
+ element name=gid
+attribute name=start
+ ref name=unsignedInt/
+/attribute
+attribute name=target
+ ref name=unsignedInt/
+/attribute
+attribute name=count
+ ref name=unsignedInt/
+/attribute
+ /element
+/zeroOrMore
+ /define
!--
Resources usage defines the amount of memory (maximum and possibly
current usage) and number of virtual CPUs used by that domain.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ad5550c..a3c5c84 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1939,6 +1939,9 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainTPMDefFree(def-tpm);
+VIR_FREE(def-idmap.uidmap);
+VIR_FREE(def-idmap.gidmap);
+
VIR_FREE(def-os.type);
VIR_FREE(def-os.machine);
VIR_FREE(def-os.init);
@@ -10057,6 +10060,40 @@ cleanup:
return ret;
}
+
+/* Parse the XML definition for user namespace id map.
+ *
+ * idmap has the form of
+ *
+ * uid start='0' target='1000' count='10'/
+ * gid start='0' target='1000' count='10'/
+ */
+static virDomainIdMapEntryPtr
+virDomainIdmapDefParseXML(const xmlNodePtr *node,
+ xmlXPathContextPtr ctxt,
+ ssize_t num)
+{
+int i;
+virDomainIdMapEntryPtr idmap = NULL;
+xmlNodePtr save_ctxt = ctxt-node;
+
+if (VIR_ALLOC_N(idmap, num) 0) {
+virReportOOMError();
+goto error;
+}
+
+for (i = 0; i num; i++) {
+ctxt-node = node[i];
+virXPathUInt(string(./@start), ctxt, idmap[i].start);
+virXPathUInt(string(./@target), ctxt, idmap[i].target);
+virXPathUInt(string(./@count), ctxt, idmap[i].count);
+}
+ error:
+ctxt-node = save_ctxt;
+return idmap;
+}
+
+
/* Parse the XML definition for a vcpupin or emulatorpin.
*
* vcpupin has the form of
@@ -11804,6 +11841,43 @@ virDomainDefParseXML(xmlDocPtr xml,
}
VIR_FREE(nodes);
+/* analysis of the user namespace mapping */
+def-idmap.nuidmap = 0;
+def-idmap.uidmap = NULL;
+if ((n = virXPathNodeSet(./idmap/uid, ctxt, nodes)) 0)
+goto error;
+
+if (n) {
+def-idmap.uidmap = virDomainIdmapDefParseXML(nodes, ctxt, n);
+if (!def-idmap.uidmap)
+goto error;
+
+def-idmap.nuidmap = n;
+}
+VIR_FREE(nodes);
+
+def-idmap.ngidmap = 0;
+def-idmap.gidmap = NULL;
+
+if ((n = virXPathNodeSet(./idmap/gid, ctxt, nodes)) 0)
+goto error;
+
+
Re: [libvirt] [PATCH v3 08/12] LXC: controller: change the owner of tty devices to the root user of container
Hi!
- Ursprüngliche Mail -
Since these tty devices will be used by container,
the owner of them should be the root user of container.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_controller.c | 43
+--
1 file changed, 37 insertions(+), 6 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 7d10660..4660f25 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1380,13 +1380,14 @@ static int lxcSetPersonality(virDomainDefPtr
def)
* *TTYNAME. Heavily borrowed from glibc, but doesn't require that
* devpts == /dev/pts */
static int
-lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName)
+lxcCreateTty(virLXCControllerPtr ctrl, int *ttymaster,
+ char **ttyName, char **ttyHostPath)
{
int ret = -1;
int ptyno;
int unlock = 0;
-if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) 0)
+if ((*ttymaster = open(ctrl-devptmx,
O_RDWR|O_NOCTTY|O_NONBLOCK)) 0)
goto cleanup;
if (ioctl(*ttymaster, TIOCSPTLCK, unlock) 0)
@@ -1407,6 +1408,13 @@ lxcCreateTty(char *ptmx, int *ttymaster, char
**ttyName)
goto cleanup;
}
+if (virAsprintf(ttyHostPath, /%s/%s.devpts/%d, LXC_STATE_DIR,
+ctrl-def-name, ptyno) 0) {
+virReportOOMError();
+errno = ENOMEM;
+goto cleanup;
+}
+
ret = 0;
cleanup:
@@ -1552,18 +1560,41 @@
virLXCControllerSetupConsoles(virLXCControllerPtr ctrl,
char **containerTTYPaths)
{
size_t i;
+int ret = -1;
+uid_t uid = (uid_t)-1;
+gid_t gid = (gid_t)-1;
+char *ttyHostPath = NULL;
+
+if (ctrl-def-idmap.uidmap) {
+uid = ctrl-def-idmap.uidmap[0].target;
+gid = ctrl-def-idmap.gidmap[0].target;
+}
for (i = 0; i ctrl-nconsoles; i++) {
VIR_DEBUG(Opening tty on private %s, ctrl-devptmx);
-if (lxcCreateTty(ctrl-devptmx,
+if (lxcCreateTty(ctrl,
ctrl-consoles[i].contFd,
- containerTTYPaths[i]) 0) {
+ containerTTYPaths[i], ttyHostPath) 0) {
virReportSystemError(errno, %s,
_(Failed to allocate tty));
-return -1;
+goto out;
}
+
+/* Change the owner of tty device to the root user of
container */
+if (chown(ttyHostPath, uid, gid) 0) {
+virReportSystemError(errno,
+ _(Failed to change owner of tty
+ %s to %u:%u),
+ ttyHostPath, uid, gid);
+goto out;
+}
+VIR_FREE(ttyHostPath);
Why do you free ttyHostPath here?
You already do it in the exit path.
}
-return 0;
+
+ret = 0;
+out:
+VIR_FREE(ttyHostPath);
Double free?
+return ret;
}
Thanks,
//richard
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/12] LXC: controller: change the owner of devices created on host
Hi!
- Ursprüngliche Mail -
Since these devices are created for the container.
the owner should be the root user of the container.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_controller.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index f892ce3..b2ace20 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1260,6 +1260,8 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;
char *path = NULL;
+uid_t uid = (uid_t)-1;
+gid_t gid = (gid_t)-1;
const struct {
int maj;
int min;
@@ -1276,6 +1278,11 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
if (virLXCControllerSetupDev(ctrl) 0)
goto out;
+if (ctrl-def-idmap.uidmap) {
+uid = ctrl-def-idmap.uidmap[0].target;
+gid = ctrl-def-idmap.gidmap[0].target;
+}
+
/* Populate /dev/ with a few important bits */
for (i = 0 ; i ARRAY_CARDINALITY(devs) ; i++) {
if (virAsprintf(path, /%s/%s.dev/%s,
@@ -1293,6 +1300,13 @@ static int
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
devs[i].path);
goto out;
}
+
+if (chown(path, uid, gid) 0) {
+virReportSystemError(errno,
+ _(Failed to change owner of %s to
%u:%u),
+ devs[i].path, uid, gid);
+goto out;
+}
VIR_FREE(path);
This looks suspicious.
If you free path in the exit path you end up with a double free.
If not you may leak memory if chown() fails.
}
Thanks,
//richard
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 11/12] LXC: controller: change the owner of /dev/pts and ptmx to the root of container
Hi!
- Ursprüngliche Mail -
This two files are created for container,
the owner should be the root user of container.
Signed-off-by: Gao feng gaof...@cn.fujitsu.com
---
src/lxc/lxc_controller.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index b2ace20..7d27135 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1506,8 +1506,15 @@
virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
char *opts = NULL;
char *devpts = NULL;
char *path = NULL;
+uid_t uid = (uid_t)-1;
+gid_t gid = (gid_t)-1;
int ret = -1;
+if (ctrl-def-idmap.uidmap) {
+uid = ctrl-def-idmap.uidmap[0].target;
+gid = ctrl-def-idmap.gidmap[0].target;
+}
+
You're using this pattern a few times.
Maybe it's worth a helper function.
Thanks,
//richard
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
