Re: [libvirt] [PATCH v4 10/10] Wait for iommmu device to go away before reprobing the host driver
On Fri, Nov 20, 2015 at 9:07 PM, Laine Stump wrote:
> On 11/14/2015 03:39 AM, Shivaprasad G Bhat wrote:
>>
>> There could be a delay of 1 or 2 seconds before the vfio-pci driver
>> is unbound and the device file /dev/vfio/ is actually
>> removed. If the file exists, the host driver probing the device
>> can lead to crash. So, wait and avoid the crash.
>
>
> If this is true, it is a kernel bug and must be fixed, not glossed over with
> a clunky timed loop.
>
> In a discussion on IRC, Alex has told me that by the time you return from
> unbinding the last device from vfio-pci (by writing the ASCII representation
> of the device's PCI address to its driver/unbind in sysfs), it should be
> safe to reprobe, and the reprobe should be successful. In other words, this
> wait should be unnecessary.
>
> If libvirt added fixups like this for every transient kernel bug, it would
> end up being a fragile unmaintainable mess understood by nobody. Instead, we
> should push for the kernel to have its bugs fixed.
>
Hi Laine,
I got it confirmed from Alexey(CC'ed) that this delay is actually not needed.
So, I feel this patch and Patch 9 which unbinds from vfio in a way to
help this wait
to take place, both can be dropped. Andrea's approach of unbind can be
taken which is rather clean.
My patch http://www.redhat.com/archives/libvir-list/2015-November/msg7.html
still fixes a genuine issue which I'll rework after Andrea's series
gets merged.
Andrea,
I think we can use the mock changes and the test case(with some
change) from this series
for your series. Please let me know if you can pull them along or I'll
rework and send them after
you series is merged.
Thanks and Regards,
Shivaprasad
>>
>> This cant be tested with the mock as the delay cant be simulated.
>> The mock changes here are to just let the test cases pass for access()
>> call to check if the /dev/vfio/ exists.
>>
>> Signed-off-by: Shivaprasad G Bhat
>> ---
>> src/util/virpci.c | 41 +
>> tests/virpcimock.c | 11 ++-
>> 2 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index 5462cd2..4765302 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
>> @@ -1098,6 +1098,43 @@ virPCIIsKnownStub(char *driver)
>> return ret;
>> }
>> +#define VFIO_UNBIND_TIMEOUT 100
>> +
>> +/* It is not safe to initiate host driver probe if the vfio driver has
>> not
>> + * completely unbound the device. Usual wait time is 1 to 2 seconds.
>> + * So, return if the unbind didn't complete in 10 seconds.
>> + */
>> +static int
>> +virPCIWaitForVFIOUnbindCompletion(virPCIDevicePtr dev)
>> +{
>> +int retry = 0;
>> +int ret = -1;
>> +char *path = NULL;
>> +
>> +if (!(path = virPCIDeviceGetIOMMUGroupDev(dev)))
>> +goto cleanup;
>> +
>> +while (retry++ < VFIO_UNBIND_TIMEOUT) {
>> +if (!virFileExists(path))
>> +break;
>> + usleep(10); /* Sleep for 1/10th of a second */
>> +}
>> +
>> +if (virFileExists(path)) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("VFIO unbind not completed even after %d
>> seconds"
>> + " for device %s"), retry, dev->name);
>> +goto cleanup;
>> +}
>> +
>> +ret = 0;
>> + cleanup:
>> +VIR_FREE(path);
>> +return ret;
>> +
>> +}
>> +
>> +
>> static int
>> virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev,
>> const char *driver,
>> @@ -1288,6 +1325,10 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
>> /* This device is the last to unbind from vfio. As we explicitly
>>* add a missing device in the list to inactiveList, we will
>> just
>>* go through the list. */
>> +
>> +if (virPCIWaitForVFIOUnbindCompletion(dev) < 0)
>> +goto cleanup;
>> +
>> while (inactiveDevs && (i <
>> virPCIDeviceListCount(inactiveDevs))) {
>> virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs,
>> i);
>> if (dev->iommuGroup == pcidev->iommuGroup) {
>> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
>> index 837976a..cf92c58 100644
>> --- a/tests/virpcimock.c
>> +++ b/tests/virpcimock.c
>> @@ -52,6 +52,7 @@ static DIR * (*realopendir)(const char *name);
>> char *fakesysfsdir;
>> # define PCI_SYSFS_PREFIX "/sys/bus/pci/"
>> +# define ROOT_DEV_PREFIX "/dev/"
>> # define STDERR(...)
>> \
>> fprintf(stderr, "%s %zu: ", __FUNCTION__, (size_t) __LINE__);
>> \
>> @@ -248,6 +249,13 @@ getrealpath(char **newpath,
>> errno = ENOMEM;
>> return -1;
>> }
>> +} else if (STRPREFIX(path, ROOT_DEV_PREFIX)) {
>> +if (virAsprintfQuiet(newpath, "%s/%s",
>> + fakesysfsdir,
>> + path) < 0) {
>> +errno = ENOMEM;
>> +return -1;
>> +}
>> } else {
>>
[libvirt] [PATCH 06/10] tests: skip regeneration for problematic test
While parsing qemu command line, we don't support parsing everything and
some staff cannot be parsed out of the command line. Skip regenerating
expected output for those tests.
Signed-off-by: Pavel Hrdina
---
tests/qemuargv2xmltest.c | 13 -
tests/testutils.c| 36
tests/testutils.h| 2 ++
3 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index fc18b55..0fb382b 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -35,6 +35,12 @@ static int blankProblemElements(char *data)
return 0;
}
+static bool
+skipProblemTest(char *data)
+{
+return virtTestFindLineRegex("", data);
+}
+
typedef enum {
FLAG_EXPECT_WARNING = 1 << 0,
} virQemuXML2ArgvTestFlags;
@@ -96,7 +102,12 @@ static int testCompareXMLToArgvFiles(const char *xml,
goto fail;
if (STRNEQ(expectxml, actualxml)) {
-virtTestDifferenceFull(stderr, expectxml, xml, actualxml, NULL);
+if (skipProblemTest(expectxml) || skipProblemTest(actualxml)) {
+virtTestDifferenceFullNoRegenerate(stderr, expectxml, xml,
+ actualxml, NULL);
+} else {
+virtTestDifferenceFull(stderr, expectxml, xml, actualxml, NULL);
+}
goto fail;
}
diff --git a/tests/testutils.c b/tests/testutils.c
index 6645d61..3477ff2 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -910,6 +910,42 @@ int virtTestMain(int argc,
}
+bool
+virtTestFindLineRegex(const char *pattern,
+ char *str)
+{
+regex_t reg;
+char *lineStart = str;
+char *lineEnd = strchr(str, '\n');
+bool found = false;
+
+if (regcomp(®, pattern, REG_EXTENDED | REG_NOSUB) != 0)
+return false;
+
+while (lineStart) {
+int ret;
+
+ret = regexec(®, lineStart, 0, NULL, 0);
+/* pattern found */
+if (ret == 0) {
+lineStart = NULL;
+found = true;
+} else {
+if (lineEnd) {
+lineStart = lineEnd + 1;
+lineEnd = strchr(lineStart, '\n');
+} else {
+lineStart = NULL;
+}
+}
+}
+
+regfree(®);
+
+return found;
+}
+
+
int virtTestClearLineRegex(const char *pattern,
char *str)
{
diff --git a/tests/testutils.h b/tests/testutils.h
index 3bd9004..d4913ab 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -54,6 +54,8 @@ int virtTestRun(const char *title,
int virtTestLoadFile(const char *file, char **buf);
int virtTestCaptureProgramOutput(const char *const argv[], char **buf, int
maxlen);
+bool virtTestFindLineRegex(const char *pattern,
+ char *string);
int virtTestClearLineRegex(const char *pattern,
char *string);
--
2.6.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 09/10 v2] device: cleanup input device code
The current code was a little bit odd. At first we've removed all
possible implicit input devices from domain definition to add them later
back if there was any graphics device defined while parsing XML
description. That's not all, while formating domain definition to XML
description we at first ignore any input devices with bus different to
USB and VIRTIO and few lines later we add implicit input devices to XML.
This seems to me as a lot of code for nothing. This patch may look
to be more complicated than original approach, but this is a preferred
way to modify/add driver specific staff only in those drivers and not
deal with them in common parsing/formating functions.
The update is to add those implicit input devices into config XML to
follow the real HW configuration visible by guest OS.
There was also inconsistence between our behavior and QEMU's in the way,
that in QEMU there is no way how to disable those implicit input devices
for x86 architecture and they are available always, even without graphics
device. This applies also to XEN hypervisor. VZ driver already does its
part by putting correct implicit devices into live XML.
Signed-off-by: Pavel Hrdina
---
new in v2:
- input devices are added into offline XML
src/Makefile.am| 4 +--
src/conf/domain_conf.c | 77 ++
src/libxl/libxl_domain.c | 5 +++
src/qemu/qemu_domain.c | 22 +
src/xen/xen_driver.c | 5 +++
src/xenapi/xenapi_driver.c | 5 +++
src/xenconfig/xen_common.c | 22 +
src/xenconfig/xen_common.h | 2 ++
8 files changed, 66 insertions(+), 76 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am
index 7219f7c..7c8869a 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1210,7 +1210,7 @@ libvirt_driver_xen_impl_la_CFLAGS =
\
-I$(srcdir)/xenconfig \
$(AM_CFLAGS)
libvirt_driver_xen_impl_la_LDFLAGS = $(AM_LDFLAGS)
-libvirt_driver_xen_impl_la_LIBADD = $(XEN_LIBS)
+libvirt_driver_xen_impl_la_LIBADD = $(XEN_LIBS) libvirt_xenconfig.la
libvirt_driver_xen_impl_la_SOURCES = $(XEN_DRIVER_SOURCES)
endif WITH_XEN
@@ -1271,7 +1271,7 @@ if WITH_XENAPI
noinst_LTLIBRARIES += libvirt_driver_xenapi.la
libvirt_la_BUILT_LIBADD += libvirt_driver_xenapi.la
libvirt_driver_xenapi_la_CFLAGS = $(LIBXENSERVER_CFLAGS) $(CURL_CFLAGS) \
- -I$(srcdir)/conf $(AM_CFLAGS)
+ -I$(srcdir)/conf -I$(srcdir)/xenconfig $(AM_CFLAGS)
libvirt_driver_xenapi_la_LDFLAGS = $(AM_LDFLAGS)
libvirt_driver_xenapi_la_LIBADD = $(LIBXENSERVER_LIBS) $(CURL_LIBS)
libvirt_driver_xenapi_la_SOURCES = $(XENAPI_DRIVER_SOURCES)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5200c27..95d471b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15915,27 +15915,6 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
}
-/* With QEMU / KVM / Xen graphics, mouse + PS/2 is implicit
- * with graphics, so don't store it.
- * XXX will this be true for other virt types ? */
-if ((def->os.type == VIR_DOMAIN_OSTYPE_HVM &&
- input->bus == VIR_DOMAIN_INPUT_BUS_PS2 &&
- (input->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ||
- input->type == VIR_DOMAIN_INPUT_TYPE_KBD)) ||
-(def->os.type == VIR_DOMAIN_OSTYPE_XEN &&
- input->bus == VIR_DOMAIN_INPUT_BUS_XEN &&
- (input->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ||
- input->type == VIR_DOMAIN_INPUT_TYPE_KBD)) ||
-(def->os.type == VIR_DOMAIN_OSTYPE_EXE &&
- (def->virtType == VIR_DOMAIN_VIRT_VZ ||
- def->virtType == VIR_DOMAIN_VIRT_PARALLELS) &&
- input->bus == VIR_DOMAIN_INPUT_BUS_PARALLELS &&
- (input->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ||
- input->type == VIR_DOMAIN_INPUT_TYPE_KBD))) {
-virDomainInputDefFree(input);
-continue;
-}
-
def->inputs[def->ninputs++] = input;
}
VIR_FREE(nodes);
@@ -15956,29 +15935,6 @@ virDomainDefParseXML(xmlDocPtr xml,
}
VIR_FREE(nodes);
-/* If graphics are enabled, there's an implicit PS2 mouse */
-if (def->ngraphics > 0 &&
-(ARCH_IS_X86(def->os.arch) || def->os.arch == VIR_ARCH_NONE)) {
-int input_bus = VIR_DOMAIN_INPUT_BUS_XEN;
-
-if (def->os.type == VIR_DOMAIN_OSTYPE_HVM)
-input_bus = VIR_DOMAIN_INPUT_BUS_PS2;
-if (def->os.type == VIR_DOMAIN_OSTYPE_EXE &&
-(def->virtType == VIR_DOMAIN_VIRT_VZ ||
- def->virtType == VIR_DOMAIN_VIRT_PARALLELS))
-input_bus = VIR_DOMAIN_INPUT_BUS_PARALLELS;
-
-if (virDomainDefMaybeAddInput(def,
- VIR_DOMAIN_INPUT_TYPE_MOUSE,
- input_bus) < 0)
-goto error;
-
-if (virDomainDefMaybeAddInput(d
[libvirt] [PATCH 03/10] tests.testutils: use VIR_TEST_REGENERATE_OUTPUT for virTestDifferenceFull
This patch enable regeneration of expected output file for
virTestDifferenceFull. It also introduces new
virTestDifferenceFullNoRegenerate function for special cases, where we
don't want to regenerate output.
Signed-off-by: Pavel Hrdina
---
tests/qemuxml2xmltest.c | 8 --
tests/testutils.c | 76 +++--
tests/testutils.h | 5
3 files changed, 77 insertions(+), 12 deletions(-)
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index f967ceb..2312f72 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -188,9 +188,11 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
}
if (STRNEQ(actual, expect)) {
-virtTestDifferenceFull(stderr,
- expect, data->outActiveName,
- actual, data->inName);
+/* For status test we don't want to regenerate output to not
+ * add the status data.*/
+virtTestDifferenceFullNoRegenerate(stderr,
+ expect, data->outActiveName,
+ actual, data->inName);
goto cleanup;
}
diff --git a/tests/testutils.c b/tests/testutils.c
index 2b0d3b6..70e3456 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -440,16 +440,19 @@ virtTestCaptureProgramOutput(const char *const argv[]
ATTRIBUTE_UNUSED,
* @param expectName: name designator of the expected text
* @param actual: actual output text
* @param actualName: name designator of the actual text
+ * @param regenerate: enable or disable regenerate functionality
*
* Display expected and actual output text, trimmed to first and last
* characters at which differences occur. Displays names of the text strings if
* non-NULL.
*/
-int virtTestDifferenceFull(FILE *stream,
- const char *expect,
- const char *expectName,
- const char *actual,
- const char *actualName)
+static int
+virtTestDifferenceFullInternal(FILE *stream,
+ const char *expect,
+ const char *expectName,
+ const char *actual,
+ const char *actualName,
+ bool regenerate)
{
const char *expectStart;
const char *expectEnd;
@@ -466,6 +469,12 @@ int virtTestDifferenceFull(FILE *stream,
actualStart = actual;
actualEnd = actual + (strlen(actual)-1);
+if (regenerate && virTestGetRegenerate() > 0) {
+if (expectName && actual &&
+virFileWriteStr(expectName, actual, 0666) < 0)
+return -1;
+}
+
if (!virTestGetDebug())
return 0;
@@ -511,16 +520,65 @@ int virtTestDifferenceFull(FILE *stream,
/**
* @param stream: output stream to write differences to
* @param expect: expected output text
+ * @param expectName: name designator of the expected text
+ * @param actual: actual output text
+ * @param actualName: name designator of the actual text
+ *
+ * Display expected and actual output text, trimmed to first and last
+ * characters at which differences occur. Displays names of the text strings if
+ * non-NULL. If VIR_TEST_REGENERATE_OUTPUT is used, this function will
+ * regenerate the expected file.
+ */
+int
+virtTestDifferenceFull(FILE *stream,
+ const char *expect,
+ const char *expectName,
+ const char *actual,
+ const char *actualName)
+{
+return virtTestDifferenceFullInternal(stream, expect, expectName,
+ actual, actualName, true);
+}
+
+/**
+ * @param stream: output stream to write differences to
+ * @param expect: expected output text
+ * @param expectName: name designator of the expected text
+ * @param actual: actual output text
+ * @param actualName: name designator of the actual text
+ *
+ * Display expected and actual output text, trimmed to first and last
+ * characters at which differences occur. Displays names of the text strings if
+ * non-NULL. If VIR_TEST_REGENERATE_OUTPUT is used, this function will not
+ * regenerate the expected file.
+ */
+int
+virtTestDifferenceFullNoRegenerate(FILE *stream,
+ const char *expect,
+ const char *expectName,
+ const char *actual,
+ const char *actualName)
+{
+return virtTestDifferenceFullInternal(stream, expect, expectName,
+ actual, actualName, false);
+}
+
+/**
+ * @param stream: output stream to write differences to
+ * @param expect: expected output text
* @param actual: actual output text
*
* Display expected and actual output text, trimmed to
* first and last characters at which difference
[libvirt] [PATCH 05/10] tests: use virtTestDifferenceFull in tests where we have output file
This will enable regenerate functionality for those tests to make
developer lives easier while updating tests.
Signed-off-by: Pavel Hrdina
---
tests/domainsnapshotxml2xmltest.c | 2 +-
tests/interfacexml2xmltest.c | 2 +-
tests/lxcconf2xmltest.c | 2 +-
tests/nodedevxml2xmltest.c| 2 +-
tests/qemuargv2xmltest.c | 7 ---
tests/qemuhotplugtest.c | 11 ---
tests/vboxsnapshotxmltest.c | 2 +-
7 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/tests/domainsnapshotxml2xmltest.c
b/tests/domainsnapshotxml2xmltest.c
index cd91cfa..cf91447 100644
--- a/tests/domainsnapshotxml2xmltest.c
+++ b/tests/domainsnapshotxml2xmltest.c
@@ -114,7 +114,7 @@ testCompareXMLToXMLFiles(const char *inxml,
}
if (STRNEQ(outXmlData, actual)) {
-virtTestDifference(stderr, outXmlData, actual);
+virtTestDifferenceFull(stderr, outXmlData, outxml, actual, inxml);
goto cleanup;
}
diff --git a/tests/interfacexml2xmltest.c b/tests/interfacexml2xmltest.c
index 65f5167..ba34746 100644
--- a/tests/interfacexml2xmltest.c
+++ b/tests/interfacexml2xmltest.c
@@ -34,7 +34,7 @@ testCompareXMLToXMLFiles(const char *xml)
goto fail;
if (STRNEQ(xmlData, actual)) {
-virtTestDifference(stderr, xmlData, actual);
+virtTestDifferenceFull(stderr, xmlData, xml, actual, NULL);
goto fail;
}
diff --git a/tests/lxcconf2xmltest.c b/tests/lxcconf2xmltest.c
index ed21e8a..fd5bc03 100644
--- a/tests/lxcconf2xmltest.c
+++ b/tests/lxcconf2xmltest.c
@@ -51,7 +51,7 @@ testCompareXMLToConfigFiles(const char *xml,
goto fail;
if (STRNEQ(expectxml, actualxml)) {
-virtTestDifference(stderr, expectxml, actualxml);
+virtTestDifferenceFull(stderr, expectxml, xml, actualxml, NULL);
goto fail;
}
}
diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
index a37d729..0089b5d 100644
--- a/tests/nodedevxml2xmltest.c
+++ b/tests/nodedevxml2xmltest.c
@@ -34,7 +34,7 @@ testCompareXMLToXMLFiles(const char *xml)
goto fail;
if (STRNEQ(xmlData, actual)) {
-virtTestDifference(stderr, xmlData, actual);
+virtTestDifferenceFull(stderr, xmlData, xml, actual, NULL);
goto fail;
}
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 7759a09..fc18b55 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -90,12 +90,13 @@ static int testCompareXMLToArgvFiles(const char *xml,
if (!(actualxml = virDomainDefFormat(vmdef, 0)))
goto fail;
-if (blankProblemElements(expectxml) < 0 ||
-blankProblemElements(actualxml) < 0)
+if (!virTestGetRegenerate() &&
+(blankProblemElements(expectxml) < 0 ||
+ blankProblemElements(actualxml) < 0))
goto fail;
if (STRNEQ(expectxml, actualxml)) {
-virtTestDifference(stderr, expectxml, actualxml);
+virtTestDifferenceFull(stderr, expectxml, xml, actualxml, NULL);
goto fail;
}
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 102e052..61ade25 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -175,6 +175,7 @@ testQemuHotplugUpdate(virDomainObjPtr vm,
static int
testQemuHotplugCheckResult(virDomainObjPtr vm,
const char *expected,
+ const char *expectedFile,
bool fail)
{
char *actual;
@@ -192,7 +193,9 @@ testQemuHotplugCheckResult(virDomainObjPtr vm,
ret = 0;
} else {
if (!fail)
-virtTestDifference(stderr, expected, actual);
+virtTestDifferenceFull(stderr,
+ expected, expectedFile,
+ actual, NULL);
ret = -1;
}
@@ -294,13 +297,15 @@ testQemuHotplug(const void *data)
VIR_FREE(dev);
}
if (ret == 0 || fail)
-ret = testQemuHotplugCheckResult(vm, result_xml, fail);
+ret = testQemuHotplugCheckResult(vm, result_xml,
+ result_filename, fail);
break;
case DETACH:
ret = testQemuHotplugDetach(vm, dev);
if (ret == 0 || fail)
-ret = testQemuHotplugCheckResult(vm, domain_xml, fail);
+ret = testQemuHotplugCheckResult(vm, domain_xml,
+ domain_filename, fail);
break;
case UPDATE:
diff --git a/tests/vboxsnapshotxmltest.c b/tests/vboxsnapshotxmltest.c
index 4ee7537..87b8571 100644
--- a/tests/vboxsnapshotxmltest.c
+++ b/tests/vboxsnapshotxmltest.c
@@ -85,7 +85,7 @@ testCompareXMLtoXMLFiles(const char *xml)
goto cleanup;
if (STRNEQ(actual, xmlData)) {
-virtTestDifference(stderr, xmlData, actual);
+virtTestDifferenceFull(stderr, xmlData, xml, actual, NULL);
goto cl
[libvirt] [PATCH 01/10] xen: move virDomainDefPostParse to xenParseSxpr
This patch partially reverts previous commit 91a00424 and moves the post
parse function to xenParseSxpr. This update is required because xen
driver calls xenParseSxpr directly.
Signed-off-by: Pavel Hrdina
---
src/xen/xend_internal.c | 6 --
src/xenconfig/xen_sxpr.c | 22 +++---
src/xenconfig/xen_sxpr.h | 9 +++--
3 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index 21d99e3..a2f072b 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -1595,7 +1595,9 @@ xenDaemonDomainFetch(virConnectPtr conn, int domid, const
char *name,
priv->xendConfigVersion,
cpus,
tty,
- vncport)))
+ vncport,
+ priv->caps,
+ priv->xmlopt)))
goto cleanup;
cleanup:
@@ -3228,7 +3230,7 @@ xenDaemonDomainBlockPeek(virConnectPtr conn,
xenUnifiedUnlock(priv);
if (!(def = xenParseSxpr(root, priv->xendConfigVersion, NULL, tty,
- vncport)))
+ vncport, priv->caps, priv->xmlopt)))
goto cleanup;
if (!(actual = virDomainDiskPathByName(def, path))) {
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c
index 49f438d..e69e91f 100644
--- a/src/xenconfig/xen_sxpr.c
+++ b/src/xenconfig/xen_sxpr.c
@@ -1087,7 +1087,11 @@ xenParseSxprPCI(virDomainDefPtr def,
virDomainDefPtr
xenParseSxpr(const struct sexpr *root,
int xendConfigVersion,
- const char *cpus, char *tty, int vncport)
+ const char *cpus,
+ char *tty,
+ int vncport,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt)
{
const char *tmp;
virDomainDefPtr def;
@@ -1475,6 +1479,10 @@ xenParseSxpr(const struct sexpr *root,
goto error;
}
+if (virDomainDefPostParse(def, caps, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE,
+ xmlopt) < 0)
+goto error;
+
return def;
error:
@@ -1511,16 +1519,8 @@ xenParseSxprString(const char *sexpr,
if (!root)
return NULL;
-if (!(def = xenParseSxpr(root, xendConfigVersion, NULL, tty, vncport)))
-goto cleanup;
-
-if (virDomainDefPostParse(def, caps, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE,
- xmlopt) < 0) {
-virDomainDefFree(def);
-def = NULL;
-}
-
- cleanup:
+def = xenParseSxpr(root, xendConfigVersion, NULL, tty, vncport,
+ caps, xmlopt);
sexpr_free(root);
return def;
diff --git a/src/xenconfig/xen_sxpr.h b/src/xenconfig/xen_sxpr.h
index e42a931..36f1d1a 100644
--- a/src/xenconfig/xen_sxpr.h
+++ b/src/xenconfig/xen_sxpr.h
@@ -50,8 +50,13 @@ virDomainDefPtr xenParseSxprString(const char *sexpr,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt);
-virDomainDefPtr xenParseSxpr(const struct sexpr *root, int xendConfigVersion,
- const char *cpus, char *tty, int vncport);
+virDomainDefPtr xenParseSxpr(const struct sexpr *root,
+ int xendConfigVersion,
+ const char *cpus,
+ char *tty,
+ int vncport,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt);
int xenParseSxprSound(virDomainDefPtr def, const char *str);
--
2.6.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 04/10] tests.testutils: use virTestDifferenceFull in virtTestCompareToFile
Let's use the new virTestDifferenceFull function that will regenerate
the expected output and fail the test to let developer know that there
something was updated.
Signed-off-by: Pavel Hrdina
---
tests/testutils.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c
index 70e3456..6645d61 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -670,16 +670,12 @@ virtTestCompareToFile(const char *strcontent,
if (STRNEQ_NULLABLE(fixedcontent ? fixedcontent : strcontent,
filecontent)) {
-if (virTestGetRegenerate()) {
-if (virFileWriteStr(filename, strcontent, 0666) < 0)
-goto failure;
-goto out;
-}
-virtTestDifference(stderr, filecontent, strcontent);
+virtTestDifferenceFull(stderr,
+ filecontent, filename,
+ strcontent, NULL);
goto failure;
}
- out:
ret = 0;
failure:
VIR_FREE(fixedcontent);
--
2.6.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/10] virsh: Refactor the table output of the list command
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
> Clean up some duplicate code so it is easier to add new parameters. The
> tests need to be adjusted because after this patch there will be
> additional spaces at the end of lines in the output of virsh list, but
> this affects only the table output that is meant to be read by users.
>
> Signed-off-by: Martin Kletzander
> ---
> tests/virshtest.c| 14 +++---
> tools/virsh-domain-monitor.c | 38 +++---
> 2 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/tests/virshtest.c b/tests/virshtest.c
> index 3fdae92b7834..5983b5b190d6 100644
> --- a/tests/virshtest.c
> +++ b/tests/virshtest.c
> @@ -98,9 +98,9 @@ static int testCompareListDefault(const void *data
> ATTRIBUTE_UNUSED)
> {
>const char *const argv[] = { VIRSH_DEFAULT, "list", NULL };
>const char *exp = "\
> - IdName State\n\
> -\n\
> - 1 test running\n\
> + IdName State \n\
> +-\n\
> + 1 test running \n\
> \n";
>return testCompareOutputLit(exp, NULL, argv);
> }
> @@ -109,10 +109,10 @@ static int testCompareListCustom(const void *data
> ATTRIBUTE_UNUSED)
> {
>const char *const argv[] = { VIRSH_CUSTOM, "list", NULL };
>const char *exp = "\
> - IdName State\n\
> -\n\
> - 1 fv0running\n\
> - 2 fc4running\n\
> + IdName State \n\
> +-\n\
> + 1 fv0running \n\
> + 2 fc4running \n\
[1]If only to make the output line up w/r/t \n...
> \n";
>return testCompareOutputLit(exp, NULL, argv);
> }
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index db5bf4b2a566..24b902905968 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1914,16 +1914,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>
> /* print table header in legacy mode */
> if (optTable) {
> +vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"),
> _("State"));
> +
> if (optTitle)
> -vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n",
> - _("Id"), _("Name"), _("State"), _("Title"),
> - "-"
> - "-");
> -else
> -vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n",
> - _("Id"), _("Name"), _("State"),
> - "-"
> - "---");
> +vshPrintExtra(ctl, " %-20s\n", _("Title"));
^^
I believe if Title were added there'd be an extra line once the
following dashes are printed (end w/ \n, start with \n)
> +
> +vshPrintExtra(ctl, "\n%s",
> + "-");
> +
> +if (optTitle)
> +vshPrintExtra(ctl, "%s", "-");
It does seem in the expected virshtest.c output we're still off by 1.
That is there is 1 more "-" than extra space after the State column.
[1]
Instead of counting dashes...
dashcount = 45; /* Or some value - whatever it is */
...
if (optTitle) {
...
dashcount += 20;
}
,,,
for (i = 0; i < dashcount; i++)
buf[i] = '-';
vshPrintExtra(ctl, "%s\n", buf);
...
where of course buf is appropriately defined ;-) A nit would be that
the old output was 3 dashes longer.
ACK with at least the fixup as a result of having Title on the line (and
it would be nice to add a test for it too)!
Whether you implement the '-' hack is up to you, but it may make future
adjustments more simple...
John
> +
> +vshPrintExtra(ctl, "\n");
> }
>
> for (i = 0; i < list->ndomains; i++) {
> @@ -1945,23 +1947,21 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
> state = -2;
>
> if (optTable) {
> +vshPrint(ctl, " %-5s %-30s %-10s", id_buf,
> + virDomainGetName(dom),
> + state == -2 ? _("saved")
> + : virshDomainStateToString(state));
> +
> if (optTitle) {
> if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
> goto cleanup;
>
> -vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf,
> - virDomainGetName(dom),
> - state == -2 ? _("saved")
> - : virshD
Re: [libvirt] [PATCH v2 03/10] virsh: Add support for list -reason
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
> Add new parameter for the list command, --reason. This adds another
> optional column in the table output of listed domains.
>
> Signed-off-by: Martin Kletzander
> ---
> tests/virshtest.c| 16
> tools/virsh-domain-monitor.c | 19 ++-
> tools/virsh.pod | 5 -
> 3 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/tests/virshtest.c b/tests/virshtest.c
> index 5983b5b190d6..ecec898c7264 100644
> --- a/tests/virshtest.c
> +++ b/tests/virshtest.c
> @@ -117,6 +117,18 @@ static int testCompareListCustom(const void *data
> ATTRIBUTE_UNUSED)
>return testCompareOutputLit(exp, NULL, argv);
> }
>
> +static int testCompareListReason(const void *data ATTRIBUTE_UNUSED)
> +{
> +const char *const argv[] = { VIRSH_CUSTOM, "list", "--reason", NULL };
> + const char *exp = "\
> + IdName State Reason\n\
> +\n\
> + 1 fv0runningunknown \n\
> + 2 fc4runningunknown \n\
> +\n";
> + return testCompareOutputLit(exp, NULL, argv);
> +}
> +
Nice to have this test... Would be better to have one with Table too...
> static int testCompareNodeinfoDefault(const void *data ATTRIBUTE_UNUSED)
> {
>const char *const argv[] = { VIRSH_DEFAULT, "nodeinfo", NULL };
> @@ -266,6 +278,10 @@ mymain(void)
> testCompareListCustom, NULL) != 0)
> ret = -1;
>
> +if (virtTestRun("virsh list (with reason)",
> +testCompareListReason, NULL) != 0)
> +ret = -1;
> +
> if (virtTestRun("virsh nodeinfo (default)",
> testCompareNodeinfoDefault, NULL) != 0)
> ret = -1;
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index 24b902905968..e2ef2c566f84 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1848,6 +1848,10 @@ static const vshCmdOptDef opts_list[] = {
> .type = VSH_OT_BOOL,
> .help = N_("show domain title")
> },
> +{.name = "reason",
> + .type = VSH_OT_BOOL,
> + .help = N_("show domain state reason")
> +},
> {.name = NULL}
> };
>
> @@ -1862,10 +1866,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
> bool optTable = vshCommandOptBool(cmd, "table");
> bool optUUID = vshCommandOptBool(cmd, "uuid");
> bool optName = vshCommandOptBool(cmd, "name");
> +bool optReason = vshCommandOptBool(cmd, "reason");
> size_t i;
> char *title;
> char uuid[VIR_UUID_STRING_BUFLEN];
> int state;
> +int state_reason;
> bool ret = false;
> virshDomainListPtr list = NULL;
> virDomainPtr dom;
> @@ -1916,12 +1922,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
> if (optTable) {
> vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"),
> _("State"));
>
> +if (optReason)
> +vshPrintExtra(ctl, " %-10s", _("Reason"));
> +
Fairly easy to "dashcount += 10;"
or because the longest reason I can find is 19 characters, perhaps use
20... The only oddity is long state reason when title is also used. Your
call on this.
> if (optTitle)
> vshPrintExtra(ctl, " %-20s\n", _("Title"));
>
> vshPrintExtra(ctl, "\n%s",
>"-");
>
> +if (optReason)
> +vshPrintExtra(ctl, "%s", "---");
> +
not needing this if use dashcount...
> if (optTitle)
> vshPrintExtra(ctl, "%s", "-");
>
> @@ -1936,7 +1948,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
> else
> ignore_value(virStrcpyStatic(id_buf, "-"));
>
> -state = virshDomainState(ctl, dom, NULL);
> +state = virshDomainState(ctl, dom, &state_reason);
>
> /* Domain could've been removed in the meantime */
> if (state < 0)
> @@ -1952,6 +1964,11 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
> state == -2 ? _("saved")
> : virshDomainStateToString(state));
>
> +if (optReason) {
> +vshPrint(ctl, " %-10s",
> + virshDomainStateReasonToString(state,
> state_reason));
> +}
> +
> if (optTitle) {
> if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
> goto cleanup;
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 21ae4a3e5b46..7fddfc65d48c 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -393,7 +393,7 @@ value will generate output for the specific machine.
> Inject NMI to the guest.
>
> =item B [I<--inactive> | I<--all>]
> - [I<--managed-save>] [I<--title>]
> + [I<--managed-save>] [I<--title>] [I<--reason>]
>
Re: [libvirt] [PATCH v2 00/10] Make loading domains with invalid XML possible
On 12/01/2015 12:35 PM, Martin Kletzander wrote: > We always had to deal with new parsing errors in a weird way. All of > them needed to go into functions starting the domains. That messes up > the code, it's confusing to newcomers and so on. > > I once had an idea that we can handle this stuff. We know what > failed, we have the XML that failed parsing and if the problem is not > in the domain name nor UUID, we can create a domain object out of that > as well. This way we are able to do something we weren't with this > series applied. Example follows. > > Assume "dummy" is a domain with invalid XML (I just modified the > file). Now, just for the purpose of this silly example, let's say we > allowed domains with archtecture x86_*, but now we've realized that > x86_64 is the only one we want to allow, but there already is a domain > defined that has . With the current master, > the domain would be lost, so we would need to modify the funstion > starting the domain (e.g. qemuProcessStart for qemu domain). However, > with this series, it behaves like this: > > # virsh list --all --reason > IdName State Reason > --- > - dummy shut off invalid XML > > # virsh domstate --reason dummy > shut off (invalid XML) > > # virsh start dummy > error: Failed to start domain dummy > error: XML error: domain 'dummy' was not loaded due to an XML error > (unsupported configuration: Unknown architecture x86_256), please > redefine it > > # VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy > Domain dummy XML configuration edited. > > # virsh domstate --reason dummy > shut off (unknown) > > # virsh start dummy > Domain dummy started > > This is a logical next step for us to clean and separate parsing and > starting, getting rid of some old code without sacrifying compatibility > and maybe generating parser code in the future. > > v2: > - rebased on top of current master (with virdomainobjlist.c) > - only disallow starting domains with invalid definitions (change >done in v1), but allow re-defining them > - add support for "virsh list --reason" to better excercise the >feature added in this patchset > > v1: > - rebase > - don't allow starting domains with invalid state > - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html > > RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html > > > Martin Kletzander (10): > conf, virsh: Add new domain shutoff reason > virsh: Refactor the table output of the list command > virsh: Add support for list -reason > qemu: Few whitespace cleanups > conf: Extract name-parsing into its own function > conf: Extract UUID parsing into its own function > conf: Optionally keep domains with invalid XML, but don't allow > starting them > qemu: Don't lookup invalid domains unless specified otherwise > qemu: Prepare basic APIs to handle invalid defs > qemu: Load domains with invalid XML on start > > include/libvirt/libvirt-domain.h | 2 + > src/bhyve/bhyve_driver.c | 2 + > src/conf/domain_conf.c | 121 > +++ > src/conf/domain_conf.h | 7 +++ > src/conf/virdomainobjlist.c | 71 +-- > src/conf/virdomainobjlist.h | 1 + > src/libvirt_private.syms | 1 + > src/libxl/libxl_driver.c | 3 + > src/lxc/lxc_driver.c | 3 + > src/qemu/qemu_driver.c | 64 + > src/uml/uml_driver.c | 2 + > tests/virshtest.c| 30 +++--- > tools/virsh-domain-monitor.c | 60 --- > tools/virsh.pod | 5 +- > 14 files changed, 303 insertions(+), 69 deletions(-) > > -- > 2.6.3 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > Beyond the few noted spots changes look good to me. Implicit ACK for those not specifically noted. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 07/10] conf: Optionally keep domains with invalid XML, but don't allow starting them
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
> Add new parameter to virDomainObjListLoadConfig() and
> virDomainObjListLoadAllConfigs() that controls whether domains with
> invalid XML (which could not be parsed) should be kept in order not to
> lose track of them. For now, the parameter is set to false in all
> callers. Each driver can switch it to true when it is prepared to deal
> with such domains.
>
> For the domain object to be created add virDomainDefParseMinimal() that
> parses only name and UUID from the XML definition. UUID must be
> present, it will not be generated. The purpose of this function is to
> be used when all else fails, but we still want a domain object to work
> with.
>
> Also explicitly disable adding the invalid domain into the list of
> active ones, as that would render our internal structures inconsistent.
>
> Signed-off-by: Martin Kletzander
> ---
> src/bhyve/bhyve_driver.c| 2 ++
> src/conf/domain_conf.c | 36 +++
> src/conf/domain_conf.h | 7 +
> src/conf/virdomainobjlist.c | 71
> ++---
> src/conf/virdomainobjlist.h | 1 +
> src/libvirt_private.syms| 1 +
> src/libxl/libxl_driver.c| 3 ++
> src/lxc/lxc_driver.c| 3 ++
> src/qemu/qemu_driver.c | 3 ++
> src/uml/uml_driver.c| 2 ++
> 10 files changed, 125 insertions(+), 4 deletions(-)
>
[...]
> @@ -406,13 +416,57 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
> virDomainObjPtr dom;
> int autostart;
> virDomainDefPtr oldDef = NULL;
> +char *xmlStr = NULL;
> +char *xmlErr = NULL;
>
> if ((configFile = virDomainConfigFile(configDir, name)) == NULL)
> goto error;
> -if (!(def = virDomainDefParseFile(configFile, caps, xmlopt,
> - VIR_DOMAIN_DEF_PARSE_INACTIVE |
> -
> VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)))
> -goto error;
> +
> +def = virDomainDefParseFile(configFile, caps, xmlopt,
> +VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS);
> +if (!def) {
> +char *tmp = NULL;
> +
> +if (!keep_invalid)
> +goto error;
> +
> +if (VIR_STRDUP(xmlErr, virGetLastErrorMessage()) < 0)
> +goto error;
> +
> +if (virFileReadAll(configFile, 1024*1024*10, &xmlStr) < 0)
Any reason to not use MAX_CONFIG_FILE_SIZE? Cannot imagine this
failing, but I mention for consistency
ACK
John
> +goto error;
> +
> +if (!(def = virDomainDefParseMinimal(NULL, xmlStr)))
> +goto error;
> +
>
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/10] conf: Extract UUID parsing into its own function
On 12/01/2015 12:35 PM, Martin Kletzander wrote:
> Create virDomainDefParseUUID that parses only the UUID from XML
> definition. This will be used in future patch(es).
>
> Signed-off-by: Martin Kletzander
> ---
> src/conf/domain_conf.c | 64
> +++---
> 1 file changed, 45 insertions(+), 19 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index de6853a4dbd0..7a295da507c4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14446,6 +14446,49 @@ virDomainVcpuParse(virDomainDefPtr def,
> return ret;
> }
>
> +/*
> + * Parse domain's UUID, optionally generating it if missing.
> + */
> +static int
> +virDomainDefParseUUID(virDomainDefPtr def,
> + xmlXPathContextPtr ctxt,
> + bool required,
> + bool *generated)
> +{
> +char *tmp = virXPathString("string(./uuid[1])", ctxt);
> +
> +if (generated)
> +*generated = false;
> +
> +if (tmp) {
> +if (virUUIDParse(tmp, def->uuid) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("malformed uuid element"));
> +VIR_FREE(tmp);
> +return -1;
> +}
> +VIR_FREE(tmp);
> +return 0;
since you're returning anyways...
int ret;
if ((ret = virUUIDParse(...)) < 0)
virReportError(...)
VIR_FREE(tmp);
return ret;
Your code works - IDC either way.
> +}
> +
> +if (required) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Missing required UUID"));
> +return -1;
> +}
> +
> +if (virUUIDGenerate(def->uuid)) {
if (virUUIDGenerate(...) < 0)
ACK with at least this one fixed - just to conform with other error path
uses...
John
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("Failed to generate UUID"));
> +return -1;
> +}
> +
> +if (generated)
> +*generated = true;
> +
> +return 0;
> +}
> +
> static int
> virDomainDefParseName(virDomainDefPtr def,
>xmlXPathContextPtr ctxt)
> @@ -14587,25 +14630,8 @@ virDomainDefParseXML(xmlDocPtr xml,
> if (virDomainDefParseName(def, ctxt) < 0)
> goto error;
>
> -/* Extract domain uuid. If both uuid and sysinfo/system/entry/uuid
> - * exist, they must match; and if only the latter exists, it can
> - * also serve as the uuid. */
> -tmp = virXPathString("string(./uuid[1])", ctxt);
> -if (!tmp) {
> -if (virUUIDGenerate(def->uuid)) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("Failed to generate UUID"));
> -goto error;
> -}
> -uuid_generated = true;
> -} else {
> -if (virUUIDParse(tmp, def->uuid) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("malformed uuid element"));
> -goto error;
> -}
> -VIR_FREE(tmp);
> -}
> +if (virDomainDefParseUUID(def, ctxt, false, &uuid_generated) < 0)
> +goto error;
>
> /* Extract short description of domain (title) */
> def->title = virXPathString("string(./title[1])", ctxt);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] qemu: Reduce memlock limit after detaching hostdev
On 12/10/2015 12:36 PM, Andrea Bolognani wrote: > On Thu, 2015-12-10 at 12:11 -0500, John Ferlan wrote: Since the add was in qemuDomainAttachHostPCIDevice, why is the remove not in qemuDomainDetachHostPCIDevice? Doing it here would mean it's call for any host device removal - whether or not it was ever added. >>> >>> Because qemuDomainDetachHostPCIDevice() is where we ask for the device >>> to be removed from the guest, but we have to wait for it to actually >>> be removed (and for the guest definition to be updated) before changing >>> the memory locking limit. >>> >>> I guess I could move this code to qemuDomainDetachThisHostDevice(), but >>> keeping it close to where the guest definition is updated looks cleaner >>> to me. >> >> Yeah - this all got confusing as I was paging back and forth... >> "DetachDevice" and "RemoveDevice"... Then I reviewed another change for >> shmem and got even more confused. > > I know what you mean: the code could sure use some moving around to > make it a bit more symmetric. And what about the names? > > qemuDomainDetachDiskDevice() > qemuDomainDetachDeviceDiskLive() > > Ehm... > > qemuDomainDetachHostPCIDevice() > qemuDomainDetachHostDevice() > qemuDomainDetachThisHostDevice() > > Oh boy :) > >>> I also fail to see how a device that was never added could be removed, >>> could you please elaborate? >> >> Since qemuDomainRemoveHostDevice could be called for any host device >> removal and not specifically the HostPCIDevice, I was pointing out by >> calling qemuDomainAdjustMaxMemLock there you could be calling it even >> when a HostPCIDevice wasn't being removed. > > Oh, I get it now. > > Calling qemuDomainAdjustMaxMemLock() more than necessary shouldn't > really cause any harm, but adding a check for > > hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI > > around the call is even better. > > Would that address your concerns? > You mean of course before the pesky "virDomainHostdevDefFree(hostdev)"? You could perhaps do it after the qemuDomainRemovePCIHostDevice in the switch or within that qemuDomainRemovePCIHostDevice code. John >> It was clear to me when I wrote it ;-) > > Again, I know exactly what you mean ;) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] qemu: Reduce memlock limit after detaching hostdev
On Thu, 2015-12-10 at 12:11 -0500, John Ferlan wrote: > > > Since the add was in qemuDomainAttachHostPCIDevice, why is the remove > > > not in qemuDomainDetachHostPCIDevice? Doing it here would mean it's > > > call for any host device removal - whether or not it was ever added. > > > > Because qemuDomainDetachHostPCIDevice() is where we ask for the device > > to be removed from the guest, but we have to wait for it to actually > > be removed (and for the guest definition to be updated) before changing > > the memory locking limit. > > > > I guess I could move this code to qemuDomainDetachThisHostDevice(), but > > keeping it close to where the guest definition is updated looks cleaner > > to me. > > Yeah - this all got confusing as I was paging back and forth... > "DetachDevice" and "RemoveDevice"... Then I reviewed another change for > shmem and got even more confused. I know what you mean: the code could sure use some moving around to make it a bit more symmetric. And what about the names? qemuDomainDetachDiskDevice() qemuDomainDetachDeviceDiskLive() Ehm... qemuDomainDetachHostPCIDevice() qemuDomainDetachHostDevice() qemuDomainDetachThisHostDevice() Oh boy :) > > I also fail to see how a device that was never added could be removed, > > could you please elaborate? > > Since qemuDomainRemoveHostDevice could be called for any host device > removal and not specifically the HostPCIDevice, I was pointing out by > calling qemuDomainAdjustMaxMemLock there you could be calling it even > when a HostPCIDevice wasn't being removed. Oh, I get it now. Calling qemuDomainAdjustMaxMemLock() more than necessary shouldn't really cause any harm, but adding a check for hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI around the call is even better. Would that address your concerns? > It was clear to me when I wrote it ;-) Again, I know exactly what you mean ;) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] qemu: Reduce memlock limit after detaching hostdev
On 12/10/2015 10:03 AM, Andrea Bolognani wrote:
> On Wed, 2015-12-09 at 15:15 -0500, John Ferlan wrote:
>> On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
>>> We increase the limit before plugging in a PCI hostdev or a memory
>>> module because some memory might need to be locked due to eg. VFIO.
>>>
>>> Of course we should do the opposite after unplugging a device: this
>>> was already the case for memory modules, but not for hostdevs.
>>> ---
>>> src/qemu/qemu_hotplug.c | 6 ++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index a5c134a..04baeff 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -3070,6 +3070,12 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>>> networkReleaseActualDevice(vm->def, net);
>>> virDomainNetDefFree(net);
>>> }
>>> +
>>> +/* QEMU might no longer need to lock as much memory, eg. we just
>>> detached
>>> + * a VFIO device, so adjust the limit here */
>>> +if (qemuDomainAdjustMaxMemLock(vm) < 0)
>>> +VIR_WARN("Failed to adjust locked memory limit");
>>> +
>>> ret = 0;
>>>
>>>cleanup:
>>>
>>
>> Since the add was in qemuDomainAttachHostPCIDevice, why is the remove
>> not in qemuDomainDetachHostPCIDevice? Doing it here would mean it's
>> call for any host device removal - whether or not it was ever added.
>
> Because qemuDomainDetachHostPCIDevice() is where we ask for the device
> to be removed from the guest, but we have to wait for it to actually
> be removed (and for the guest definition to be updated) before changing
> the memory locking limit.
>
> I guess I could move this code to qemuDomainDetachThisHostDevice(), but
> keeping it close to where the guest definition is updated looks cleaner
> to me.
>
Yeah - this all got confusing as I was paging back and forth...
"DetachDevice" and "RemoveDevice"... Then I reviewed another change for
shmem and got even more confused.
> I also fail to see how a device that was never added could be removed,
> could you please elaborate?
Since qemuDomainRemoveHostDevice could be called for any host device
removal and not specifically the HostPCIDevice, I was pointing out by
calling qemuDomainAdjustMaxMemLock there you could be calling it even
when a HostPCIDevice wasn't being removed.
It was clear to me when I wrote it ;-)
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] process: Add virProcessGetMaxMemLock()
On 12/10/2015 11:33 AM, Andrea Bolognani wrote:
> On Wed, 2015-12-09 at 12:00 -0500, John Ferlan wrote:
>> On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
>>> @@ -788,6 +788,48 @@ virProcessSetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED,
>>> unsigned long long bytes)
>>> }
>>> #endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_MEMLOCK)) */
>>>
>>> +#if HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)
>>> +int
>>> +virProcessGetMaxMemLock(pid_t pid,
>>> +unsigned long long *bytes)
>>
>> Another option would be to return bytes...
>>
>> where at least as I read patch 3 bytes == 0 is no different than
>> original_memlock == 0 especially if arg2 is NULL
>>
>> Of course, does calling getrlimit or virProcessPrLimit() potentially
>> multiple times make sense? Does blasting those error messages each time
>> to the log, but continuing on cause confusion?
>>
>> I guess what I'm thinking about is how it's eventually used in patch 3
>> and the concept of failure because something in here fails or perhaps
>> the getrlimit *or* prlimit isn't supported on the platform...
>
> I think ignoring failures in getting the current memory locking limit is
> the right thing to do in the upper layers, as that just means we won't
> be able to restore the original limit afterwards and that's not really a
> huge problem.
>
> However, at this level, we're dealing with low-level functionality and I
> think all failures should be reported appropriately, eg. with a negative
> return value and a proper error logged.
>
>>> +{
>>> +struct rlimit rlim;
>>> +
>>> +if (!bytes)
>>> +return 0;
>>
>> Since you return 0 here if passed a NULL bytes, then I think you'd have
>> to do so in the other API
>
> What other API do you mean?
>
in the code after this:
+#else /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */
+int
+virProcessGetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED,
+unsigned long long *bytes)
>>> +
>>> +if (pid == 0) {
>>> +if (getrlimit(RLIMIT_MEMLOCK, &rlim) < 0) {
>>> +virReportSystemError(errno,
>>> + "%s",
>>
>> This one could go on previous line - no big deal other than extra line
>
> I'd rather squeeze the second and third lines together or leave it as
> it is, if that's the same to you.
>
It really doesn't matter - just seems strange to have "%s", on its own
line especially when there's space on the prior line.
>>> + _("cannot get locked memory limit"));
>>> +return -1;
>>> +}
>>> +} else {
>>> +if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, NULL, &rlim) < 0) {
>>> +virReportSystemError(errno,
>>> + _("cannot get locked memory limit "
>>> + "of process %lld"),
>>> + (long long int) pid);
>>> +return -1;
>>> +}
>>> +}
>>> +
>>> +/* virProcessSetMaxMemLock() sets both rlim_cur and rlim_max to the
>>> + * same value, so we can retrieve just rlim_max here */
>>> +*bytes = rlim.rlim_max;
>>
>> One oddball thought... what if rlim.rlim_max == 0? Is that possible? I
>> suppose only if rlim_cur == 0 too, right? Not sure it makes sense and I
>> didn't chase the syscall. It does say rlim_cur can be 0 and that
>> rlim_max must be equal or higher...
>>
>> I'm just trying to logically follow through on the thought of how 0 is
>> used in patch 3.
>
> I don't know, but I don't think we should concern ourself too much with
> that case as virProcessSetMaxMemLock(), which will be used to restore
> whatever value this function returns, ignores the value 0.
>
Fair enough...
>>> +
>>> +return 0;
>>> +}
>>> +#else /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */
>>> +int
>>> +virProcessGetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED,
>>> +unsigned long long *bytes)
>>
>> Would technically be unused if kept as is... However since the other API
>> returns 0 when value is passed as NULL, this could too.
>
> Having a function that either fails as unimplemented or succeeds
> depending on its arguments doesn't feel right to me.
>
> I see, however, that all virProcessSetMax*() functions behave this way
> already so it makes sense to change do the same for consistency's sake.
>
>
Who's to say which is the right way... But since we declare using 0
does nothing, then playing following the leader shouldn't hurt in this case.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/7] Add multiqueue support for macvtaps
On 12/10/2015 02:38 AM, Michal Privoznik wrote: Patches 1, 2, 3, 6, 7 have been ACKed in previous round. However, I did slightly change them to reflect Laine's review suggestions. Patch 4 has not been ACKed yet, patch 5 is new. Michal Privoznik (7): virNetDevMacVLanCreateWithVPortProfile: Turn vnet_hdr into flag virNetDevMacVLanTapOpen: Slightly rework virNetDevMacVLanTapOpen: Rework to support multiple FDs virNetDevMacVLanTapSetup: Rework to support multiple FDs virNetDevMacVLanTapSetup: Allow enabling of IFF_MULTI_QUEUE virNetDevMacVLanCreateWithVPortProfile: Rework to support multiple FDs qemu: Enable multiqueue for macvtaps src/lxc/lxc_process.c | 3 +- src/qemu/qemu_command.c | 65 ++-- src/qemu/qemu_command.h | 2 + src/qemu/qemu_hotplug.c | 16 ++-- src/util/virnetdevmacvlan.c | 185 ++-- src/util/virnetdevmacvlan.h | 7 +- 6 files changed, 153 insertions(+), 125 deletions(-) For some reason 5/7 and 6/7 didn't get delivered to me (although they are in the list archive). I have a suggestion for slight rewording of the function description in 5/7, which I'll list here, but otherwise ACK for the series. * Turn the IFF_VNET_HDR flag, if requested and available, make sure it's * off in the other cases. Similarly, IFF_MULTI_QUEUE is enabled if * requested. However, if requested and failed to set, it is considered a * fatal error (as opposed to @vnet_hdr). Instead maybe: "Turn on the IFF_VNET_HDR flag if requested and available, but make sure it's off otherwise. Similarly, turn on IFF_MULTI_QUEUE if requested, but if it can't be set, consider it a fatal error (rather than ignoring as with @vnet_hdr)." -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] storage sheepdog: allow to specify redundancy level
On 11/19/2015 03:44 AM, Vasiliy Tolstov wrote:
> Some storage backends allows to specify per volume redundancy options.
> Sheepdog use x format for specify copies, and x:y format to specify
> data and parity block count.
So you may understand what redundancy is, but I don't believe it's
conveyed well through this commit message. using "x format" or "x:y
format" doesn't help me understand what the 'feature' is, how it's
expected to be used, and why you chose to implement this the way you
did. Also, when I look at the output generated I see a number. I'm not
sure why you chose a string.
Probably should also split this patch up a bit into separable components
such as adding a new field to the rng, which includes the structure
(.h), and code (.c) changes in order to parse, format, and in this case
"free" the data field. The patch would also have *new* tests to validate
the new/optional field. It would also require an adjustment to the
formatstorage.html.in. I know there are examples of previous checkins
that could be used.
After reading more closely there are at least 4 additional patches:
* One to use virStringSplit in virStorageBackendSheepdogParseNodeInfo
* One to show some sort of display output change in parsing nodeinfo
(there's a new field)
* One to use virStringSplit in virStorageBackendSheepdogParseVdiList
* One to add the new redundancy field.
Before posting please be sure to run "make check" and "make
syntax-check" for each patch. My run fails with both
storagevolxml2xmltest and storagevolschematest for make check and a lot
more failures for make syntax-check.
Finally, after going through the entire patch - I still don't seen
how/why redundancy was added. It's not used for anything other than
output. So what's the purpose of adding it? My belief is that it should
not be a character string. Rather it should be two numbers - what they
are called/named is still to be determined since I don't understand
their purpose yet.
As for order. I would make the virStringSplit changes and output
difference for NodeInfo first. Then add the new field.
>
> Signed-off-by: Alexey Tyabin
> Signed-off-by: Vasiliy Tolstov
> ---
> docs/schemas/storagevol.rng | 3 +
> src/conf/storage_conf.c | 2 +
> src/storage/storage_backend_sheepdog.c | 143
> +---
> src/util/virstoragefile.c | 4 +-
> src/util/virstoragefile.h | 2 +
> tests/storagebackendsheepdogtest.c | 104 ++--
> tests/storagevolxml2xmlin/vol-sheepdog.xml | 1 +
> tests/storagevolxml2xmlout/vol-sheepdog.xml | 1 +
> 8 files changed, 153 insertions(+), 107 deletions(-)
>
> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng
> index 7450547..068993f 100644
> --- a/docs/schemas/storagevol.rng
> +++ b/docs/schemas/storagevol.rng
> @@ -55,6 +55,9 @@
>
>
>
> +
> +
> +
Kind of a broad classification. There are examples that would show if
you're expecting a certain format that rather than be so broad, you
could expect a format in a certain manner (look at the IP addresses
perhaps for an example...)
Also what does this have to do with 'sizing'? This should have changed
the to have the following below "ref name='sizing'":
and
...
I use the ... because I'm not really clear whether using a string is the
correct final format. It would seem it should be two numbers. How those
get represented is still to be determined. At least 1 would be required
and the second would be optional.
>
>
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 9b8abea..d37c93a 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1345,6 +1345,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
> ret->target.allocation = ret->target.capacity;
> }
>
> +ret->target.redundancy = virXPathString("string(./redundancy)", ctxt);
> +
If this is really "x" or "x:y" as in two numbers, why not read in the
string, then if present use virStringSplit with a ":" as the separator.
That returns an array of strings either as list[0] = "x", list[1] = NULL
or list[0] = "x", list[1] = "y", list[2] = NULL.
>From there you can convert the strings into numbers. If of course the
returned list is bogus, then you can/should have a configuration or
format error here as well. Just carrying this around for some later
future use is well, confusing at this point.
I have a "suspicion" that a "0" redundancy is meaningless, so if you
stored as a pair of integers it may be less confusing and easier to
manipulate and use.
Where's the formatting for the redunancy output, e.g. no change in
virStorageVolDefFormat.
> ret->target.path = virXPathString("string(./target/path)", ctxt);
> if (options->formatFromString) {
> char *format = virXPathString("string(./target/forma
Re: [libvirt] [PATCH 2/6] process: Add virProcessGetMaxMemLock()
On Wed, 2015-12-09 at 12:00 -0500, John Ferlan wrote:
> On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
> > @@ -788,6 +788,48 @@ virProcessSetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED,
> > unsigned long long bytes)
> > }
> > #endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_MEMLOCK)) */
> >
> > +#if HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)
> > +int
> > +virProcessGetMaxMemLock(pid_t pid,
> > +unsigned long long *bytes)
>
> Another option would be to return bytes...
>
> where at least as I read patch 3 bytes == 0 is no different than
> original_memlock == 0 especially if arg2 is NULL
>
> Of course, does calling getrlimit or virProcessPrLimit() potentially
> multiple times make sense? Does blasting those error messages each time
> to the log, but continuing on cause confusion?
>
> I guess what I'm thinking about is how it's eventually used in patch 3
> and the concept of failure because something in here fails or perhaps
> the getrlimit *or* prlimit isn't supported on the platform...
I think ignoring failures in getting the current memory locking limit is
the right thing to do in the upper layers, as that just means we won't
be able to restore the original limit afterwards and that's not really a
huge problem.
However, at this level, we're dealing with low-level functionality and I
think all failures should be reported appropriately, eg. with a negative
return value and a proper error logged.
> > +{
> > +struct rlimit rlim;
> > +
> > +if (!bytes)
> > +return 0;
>
> Since you return 0 here if passed a NULL bytes, then I think you'd have
> to do so in the other API
What other API do you mean?
> > +
> > +if (pid == 0) {
> > +if (getrlimit(RLIMIT_MEMLOCK, &rlim) < 0) {
> > +virReportSystemError(errno,
> > + "%s",
>
> This one could go on previous line - no big deal other than extra line
I'd rather squeeze the second and third lines together or leave it as
it is, if that's the same to you.
> > + _("cannot get locked memory limit"));
> > +return -1;
> > +}
> > +} else {
> > +if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, NULL, &rlim) < 0) {
> > +virReportSystemError(errno,
> > + _("cannot get locked memory limit "
> > + "of process %lld"),
> > + (long long int) pid);
> > +return -1;
> > +}
> > +}
> > +
> > +/* virProcessSetMaxMemLock() sets both rlim_cur and rlim_max to the
> > + * same value, so we can retrieve just rlim_max here */
> > +*bytes = rlim.rlim_max;
>
> One oddball thought... what if rlim.rlim_max == 0? Is that possible? I
> suppose only if rlim_cur == 0 too, right? Not sure it makes sense and I
> didn't chase the syscall. It does say rlim_cur can be 0 and that
> rlim_max must be equal or higher...
>
> I'm just trying to logically follow through on the thought of how 0 is
> used in patch 3.
I don't know, but I don't think we should concern ourself too much with
that case as virProcessSetMaxMemLock(), which will be used to restore
whatever value this function returns, ignores the value 0.
> > +
> > +return 0;
> > +}
> > +#else /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */
> > +int
> > +virProcessGetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED,
> > +unsigned long long *bytes)
>
> Would technically be unused if kept as is... However since the other API
> returns 0 when value is passed as NULL, this could too.
Having a function that either fails as unimplemented or succeeds
depending on its arguments doesn't feel right to me.
I see, however, that all virProcessSetMax*() functions behave this way
already so it makes sense to change do the same for consistency's sake.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: Add qemuDomainAdjustMaxMemLock()
On Wed, 2015-12-09 at 12:19 -0500, John Ferlan wrote:
> On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
> > This function detects whether a domain needs RLIMIT_MEMLOCK
> > to be set, and if so, uses an appropriate value.
> >
> > It also stores the original value inside the virDomainObj for
> > the domain so that it can be later restored.
> > ---
> > src/conf/domain_conf.h | 3 +++
> > src/qemu/qemu_domain.c | 50
> >++
> > src/qemu/qemu_domain.h | 1 +
> > 3 files changed, 54 insertions(+)
>
> NB: This patch wouldn't "git am -3" apply for me - looks like you'll be
> having some merge conflicts to deal with.
Yeah, a rebase was definitely in order :)
> > +/**
> > + * qemuDomainAdjustMaxMemLock:
> > + *
> > + * @vm: domain
> > + *
> > + * Adjust the memory locking limit for the QEMU process associated to @vm,
> > in
> > + * order to comply with VFIO or architecture requirements.
> > + *
> > + * The limit will not be changed unless doing so is needed; the first time
> > + * the limit is changed, the original (default) limit is stored in @vm and
> > + * that value will be restored if qemuDomainAdjustMaxMemLock() is called
> > once
> > + * memory locking is no longer required.
> > + *
> > + * Returns: 0 on success, <0 on failure
> > + */
> > +int
> > +qemuDomainAdjustMaxMemLock(virDomainObjPtr vm)
>
> It seems this function does two things...
>
> #1 Get some original value if the domain requires mlock
> #2 Restores the original value
>
> Not sure it's best to mix the logic...
I believe you understand the rationale now.
> > +{
> > +unsigned long long bytes = 0;
> > +int ret = -1;
> > +
> > +if (qemuDomainRequiresMlock(vm->def)) {
> > +/* If this is the first time adjusting the limit, save the current
> > + * value so that we can restore it once memory locking is no longer
> > + * required */
> > +if (!vm->original_memlock) {
> > +if (virProcessGetMaxMemLock(vm->pid, &(vm->original_memlock))
> > < 0)
> > +goto out;
>
> Failure scenario 1 - I couldn't get some original value
>
> Should this really be a failure? Could fail for 4 reasons:
>
> 1. getrlimit returns fail when pid == 0
> 2. getrlimit && RLIMIT_MEMLOCK not available
> 3. prlimit returns fail
> 4. prlimit not available or in the case shown by commit id '05f79a38'
> the oddity from a mingw build.
>
> Given how the code is written today, if arg2 was NULL then a "0" would
> have been returned.
>
> So if it returns 0, then that means the "ability" to reset to some
> initial value is not available... Thus we just return skinny, dumb, and
> happy rather than failing. EG no worse than today.
>
> For archs/envs that support getting the value - sure we can support
> resetting the value...
Makes sense. Moreover, I can't really imagine a case where getting the
current memory limit would fail but setting a new one immediately
afterwards would succeed instead.
> > +}
> > +bytes = qemuDomainGetMlockLimitBytes(vm->def);
> > +} else {
> > +/* Once memory locking is no longer required, we can restore the
> > + * original, usually very low, limit */
> > +bytes = vm->original_memlock;
>
> Not sure I understand what is meant by "is no longer required" (yet - I
> haven't read forward)... However, if it never was required, then all the
> following code does nothing since original_memlock would be 0 (and
> nothing here or in future patches) sets it unless the domain requires it.
>
> IOW: How/why does a domain go from at one point requiring Mlock to not
> requiring it? It's not lock a running PPC64 domain is going to change
> it's arch type...
I believe this, too, is now clear to you.
> > +vm->original_memlock = 0;
> > +}
> > +
> > +/* Don't do anything unless we're actually setting a limit */
> > +if (bytes) {
> > +if (virProcessSetMaxMemLock(vm->pid, bytes) < 0)
>
> Failure scenario 2 - I couldn't set a new limit. E.G - in my mind the
> true failure. I couldn't set a new value.
Of course, this one should keep on being be a severe failure and abort
the whole operation.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] qemu: Add qemuDomainAdjustMaxMemLock()
On Wed, 2015-12-09 at 13:02 -0500, John Ferlan wrote:
> On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
> [...]
>
> > +}
> > +
> > +/* Don't do anything unless we're actually setting a limit */
> > +if (bytes) {
> > +if (virProcessSetMaxMemLock(vm->pid, bytes) < 0)
> > +goto out;
> > +}
>
> virProcessSetMaxMemLock aready checks:
>
>if (bytes == 0)
> return 0;
>
> So the "if (bytes)" is unnecessary
Good catch, I will remove the check and update the comment.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: align the cur_balloon too if not explicitly specified by the user
As of now, the cur_ballon is set to actual memory if not specified by the user.
When the user specified memory is not aligned the cur_balloon alone ends up
unaligned. For qemu in function qemuDomainAttachMemory(), the cur_balloon
wouldn't add up to the actual memory as the cur_ballon was not aligned.
So, there is need for explicit setmem post attach-device for such guests.
The decision as to whether to align the cur_balloon memory or not is
not possible if we set it to actual memory by default in post-parse.
Move the default cur_balloon assignment to their respective drivers during
domain start wherever possible. For qemu align the cur_balloon too iow assign
the aligned actual memory when not specified by the user.
Signed-off-by: Shivaprasad G Bhat
---
src/conf/domain_conf.c |3 +--
src/libxl/libxl_conf.c |2 ++
src/lxc/lxc_process.c |3 +++
src/openvz/openvz_driver.c | 13 +++--
src/phyp/phyp_driver.c | 10 +++---
src/qemu/qemu_domain.c |3 +++
src/uml/uml_conf.c |3 +++
src/vbox/vbox_common.c |3 +++
src/vmx/vmx.c |3 +++
src/vz/vz_sdk.c|3 +++
src/xenconfig/xen_common.c |3 +++
src/xenconfig/xen_sxpr.c |4
12 files changed, 38 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2f5c0ed..68338f4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3527,8 +3527,7 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
return -1;
}
-if (def->mem.cur_balloon > virDomainDefGetMemoryActual(def) ||
-def->mem.cur_balloon == 0)
+if (def->mem.cur_balloon > virDomainDefGetMemoryActual(def))
def->mem.cur_balloon = virDomainDefGetMemoryActual(def);
if ((def->mem.max_memory || def->mem.memory_slots) &&
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 4eed5ca..6b6e764 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -665,6 +665,8 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
}
b_info->sched_params.weight = 1000;
b_info->max_memkb = virDomainDefGetMemoryInitial(def);
+if (!def->mem.cur_balloon)
+def->mem.cur_balloon = virDomainDefGetMemoryInitial(def);
b_info->target_memkb = def->mem.cur_balloon;
if (hvm) {
char bootorder[VIR_DOMAIN_BOOT_LAST + 1];
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 57e3880..201ee61 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1257,6 +1257,9 @@ int virLXCProcessStart(virConnectPtr conn,
vm->def->resource = res;
}
+if (!vm->def->mem.cur_balloon)
+vm->def->mem.cur_balloon = virDomainDefGetMemoryActual(vm->def);
+
if (virAsprintf(&logfile, "%s/%s.log",
cfg->logDir, vm->def->name) < 0)
goto cleanup;
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index b8c0f50..8a56d94 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -1043,12 +1043,13 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const
char *xml, unsigned int fla
}
}
-if (vm->def->mem.cur_balloon > 0) {
-if (openvzDomainSetMemoryInternal(vm, vm->def->mem.cur_balloon) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Could not set memory size"));
- goto cleanup;
-}
+if (!vm->def->mem.cur_balloon)
+vm->def->mem.cur_balloon = virDomainDefGetMemoryActual(vm->def);
+
+if (openvzDomainSetMemoryInternal(vm, vm->def->mem.cur_balloon) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not set memory size"));
+goto cleanup;
}
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 14264c0..1fc7b34 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -3491,13 +3491,6 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def)
int exit_status = 0;
virBuffer buf = VIR_BUFFER_INITIALIZER;
-if (!def->mem.cur_balloon) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Field on the domain XML file is "
- "missing or has invalid value"));
-goto cleanup;
-}
-
if (!virDomainDefGetMemoryInitial(def)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("Field on the domain XML file is missing or "
@@ -3505,6 +3498,9 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def)
goto cleanup;
}
+if (!def->mem.cur_balloon)
+def->mem.cur_balloon = virDomainDefGetMemoryActual(def);
+
if (def->ndisks < 1) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("Domain XML must contain at least one
element."));
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 40e1f18..9d92d99 100644
--
[libvirt] [PATCH] Revert "admin: Rename virAdmConnect to virAdmDaemon"
Commmit df8192aa introduced admin related rename and some minor
(caused by automated approach, aka sed) and some more severe isues along with
it. First reason to revert is the inconsistency with libvirt library.
Although we deal with the daemon directly rather than with a specific
hypervisor, we still do have a connection. That being said, contributors might
get under the impression that AdmDaemonNew would spawn/start a new daemon
(since it's admin API, why not...), or AdmDaemonClose would do the exact
opposite or they might expect DaemonIsAlive report overall status of the daemon
which definitely isn't the case.
The second reason to revert this patch is renaming virt-admin client. The
client tool does not necessarily have to reflect the names of the API's it's
using in his internals. An example would be 's/vshAdmConnect/vshAdmDaemon'
where noone can be certain of what the latter function really does. The former
is quite expressive about some connection magic it performs, but the latter does
not say anything, especially when vshAdmReconnect and vshAdmDisconnect were
left untouched.
---
The admin API rename is not entirely bad idea (e.g. virAdmDaemonGetURI sounds
a little better than virAdmConnectGetURI), so by introducing this patch
I'd like to reopen the discussion about what we could/should rename
(if anything) and which parts should definitely be left untouched, because
there is still time to do somthing with it since admin is disabled and not
distributed in the package.
daemon/admin_server.c | 22 ++---
include/libvirt/libvirt-admin.h | 48 +-
src/admin/admin_protocol.x | 10 +--
src/admin/admin_remote.c| 36
src/admin_protocol-structs | 10 +--
src/datatypes.c | 38
src/datatypes.h | 28 +++---
src/libvirt-admin.c | 195
src/libvirt_admin_private.syms | 4 +-
src/libvirt_admin_public.syms | 16 ++--
src/rpc/gendispatch.pl | 2 +-
tools/virsh.c | 18 ++--
tools/virsh.h | 1 -
tools/virt-admin.c | 65 +++---
tools/virt-admin.h | 3 +-
tools/vsh.h | 1 +
16 files changed, 249 insertions(+), 248 deletions(-)
diff --git a/daemon/admin_server.c b/daemon/admin_server.c
index 678e8bc..189091e 100644
--- a/daemon/admin_server.c
+++ b/daemon/admin_server.c
@@ -79,11 +79,11 @@ remoteAdmClientInitHook(virNetServerClientPtr client
ATTRIBUTE_UNUSED,
/* Functions */
static int
-adminDispatchDaemonOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
-virNetServerClientPtr client,
-virNetMessagePtr msg ATTRIBUTE_UNUSED,
-virNetMessageErrorPtr rerr,
-struct admin_daemon_open_args *args)
+adminDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
+ struct admin_connect_open_args *args)
{
unsigned int flags;
struct daemonAdmClientPrivate *priv =
@@ -105,18 +105,18 @@ adminDispatchDaemonOpen(virNetServerPtr server
ATTRIBUTE_UNUSED,
}
static int
-adminDispatchDaemonClose(virNetServerPtr server ATTRIBUTE_UNUSED,
- virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED)
+adminDispatchConnectClose(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED)
{
virNetServerClientDelayedClose(client);
return 0;
}
static int
-adminDaemonGetVersion(virNetDaemonPtr dmn ATTRIBUTE_UNUSED,
- unsigned long long *libVer)
+adminConnectGetLibVersion(virNetDaemonPtr dmn ATTRIBUTE_UNUSED,
+ unsigned long long *libVer)
{
if (libVer)
*libVer = LIBVIR_VERSION_NUMBER;
diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index b81698e..ab9df96 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -35,51 +35,53 @@ extern "C" {
# undef __VIR_ADMIN_H_INCLUDES__
/**
- * virAdmDaemon:
+ * virAdmConnect:
*
- * a virAdmDaemon is a private structure representing a remote daemon.
+ * a virAdmConnect is a private structure representing a connection to
+ * libvirt daemon.
*/
-typedef struct _virAdmDaemon virAdmDaemon;
+typedef struct _virAdmConnect virAdmConnect;
/**
- * virAdmDaemonPtr:
+ * virAdmConnectPtr:
*
- * a virAdmDaemonPtr is pointer to a virAdmDaemon private structure,
- * this is the type used to reference a daemon in the API.
[libvirt] [PATCH v1] libvirtd: Increase NL buffer size for lots of interface
1. When switching CPUs to offline/online in a system more than 128 cpus
2. When using virsh to destroy domain in a system with more interface
All of above happens nl_recv returned with error: No buffer space available.
This patch set socket buffer size to 128K and turn on message peeking for
nl_recv,
as this would solve this problem totally and permanetly.
LTC-Bugzilla: #133359 #125768
Signed-off-by: Leno Hou
Cc: Wenyi Gao
---
src/util/virnetlink.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 679b48e..c8c9fe0 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -696,6 +696,14 @@ virNetlinkEventServiceStart(unsigned int protocol,
unsigned int groups)
goto error_server;
}
+if (nl_socket_set_buffer_size(srv->netlinknh, 131702, 0) < 0) {
+virReportSystemError(errno,
+"%s",_("cannot set netlink socket buffer size to 128k"));
+goto error_server;
+}
+
+nl_socket_enable_msg_peek(srv->netlinknh);
+
if ((srv->eventwatch = virEventAddHandle(fd,
VIR_EVENT_HANDLE_READABLE,
virNetlinkEventCallback,
--
1.9.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/8] qemu_driver: add support to perf event
This patch implement the internal driver API for perf event into
qemu driver.
Signed-off-by: Qiaowei Ren
---
include/libvirt/libvirt-domain.h | 1 +
src/qemu/qemu_domain.h | 3 +
src/qemu/qemu_driver.c | 133 +++
src/qemu/qemu_process.c | 6 ++
4 files changed, 143 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index cc1b29b..4f29f54 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1771,6 +1771,7 @@ typedef enum {
VIR_DOMAIN_STATS_VCPU = (1 << 3), /* return domain virtual CPU info */
VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */
VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */
+VIR_DOMAIN_STATS_PERF = (1 << 6), /* return domain perf event info */
} virDomainStatsTypes;
typedef enum {
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 14892fd..080498e 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -26,6 +26,7 @@
# include "virthread.h"
# include "vircgroup.h"
+# include "virperf.h"
# include "domain_addr.h"
# include "domain_conf.h"
# include "snapshot_conf.h"
@@ -191,6 +192,8 @@ struct _qemuDomainObjPrivate {
virCgroupPtr cgroup;
+virPerfPtr perf;
+
virCond unplugFinished; /* signals that unpluggingDevice was unplugged */
const char *unpluggingDevice; /* alias of the device that is being
unplugged */
char **qemuDevices; /* NULL-terminated list of devices aliases known to
QEMU */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ae1d8e7..888241f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -98,6 +98,7 @@
#include "virhostdev.h"
#include "domain_capabilities.h"
#include "vircgroup.h"
+#include "virperf.h"
#include "virnuma.h"
#include "dirname.h"
#include "network/bridge_driver.h"
@@ -10249,6 +10250,86 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
}
static int
+qemuDomainSetPerfEvents(virDomainPtr dom,
+virTypedParameterPtr params,
+int nparams)
+{
+size_t i;
+virDomainObjPtr vm = NULL;
+qemuDomainObjPrivatePtr priv;
+int ret = -1;
+virPerfEventType type;
+bool enabled;
+
+if (virTypedParamsValidate(params, nparams, VIR_PERF_PARAMETERS) < 0)
+return -1;
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return -1;
+
+priv = vm->privateData;
+
+if (virDomainSetPerfEventsEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+for (i = 0; i < nparams; i++) {
+virTypedParameterPtr param = ¶ms[i];
+enabled = params->value.b;
+type = virPerfEventTypeFromString(param->field);
+
+if (!enabled && virPerfEventDisable(priv->perf, type))
+goto cleanup;
+if (enabled && virPerfEventEnable(priv->perf, type, vm->pid))
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+virDomainObjEndAPI(&vm);
+return ret;
+}
+
+static int
+qemuDomainGetPerfEvents(virDomainPtr dom,
+virTypedParameterPtr *params,
+int *nparams)
+{
+size_t i;
+virDomainObjPtr vm = NULL;
+qemuDomainObjPrivatePtr priv;
+int ret = -1;
+virTypedParameterPtr par = NULL;
+int maxpar = 0;
+int npar = 0;
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+goto cleanup;
+
+priv = vm->privateData;
+
+if (virDomainGetPerfEventsEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+if (virTypedParamsAddBoolean(&par, &npar, &maxpar,
+ virPerfEventTypeToString(i),
+ virPerfEventIsEnabled(priv->perf, i)) <
0) {
+virTypedParamsFree(par, npar);
+goto cleanup;
+}
+}
+
+*params = par;
+*nparams = npar;
+ret = 0;
+
+ cleanup:
+virDomainObjEndAPI(&vm);
+return ret;
+}
+
+static int
qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
unsigned long long period, long long quota)
{
@@ -19427,6 +19508,55 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
#undef QEMU_ADD_COUNT_PARAM
+static int
+qemuDomainGetStatsPerfCmt(virPerfPtr perf,
+ virDomainStatsRecordPtr record,
+ int *maxparams)
+{
+uint64_t cache = 0;
+
+if (virPerfReadEvent(perf, VIR_PERF_EVENT_CMT, &cache) < 0)
+return -1;
+
+if (virTypedParamsAddULLong(&record->params,
+&record->nparams,
+maxparams,
+"perf.cache",
+cache) < 0)
+return -1;
+
+return 0;
+}
+
+static int
+qemuDomainGetStatsPerf(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+ virDomainObjPtr dom,
+
[libvirt] [PATCH 0/8] Add perf and Intel CMT feature support
The series mainly adds Intel CMT feature support into libvirt. CMT is new introduced PQos (Platform Qos) feature to monitor the usage of cache by applications running on the platform. Currently CMT patches has been merged into Linux kernel mainline. The CMT implementation in Linux kernel is based on perf mechanism and there is no support for perf in libvirt, and so this series firstly add perf support into libvirt, including two public API and a set of util interfaces. And based on these APIs and interfaces, thie series implements CMT perf event support. TODO: 1. add support for new APIs into libvirt-python library. Changes since v1: * change perf APIs implementation to match new style of the API. * add new xml element * reenable all perf events previously enabled when libvirtd daemon restart. * redesign perf util functions. Changes since v2: * add an example XML file to the test suite. * add virPerfReadEvent(). * change 'perf' xml element to new style. * change 'perf' command to new stype. Qiaowei Ren (8): perf: add new public APIs for perf event perf: implement the remote protocol for perf event perf: implement a set of util functions for perf event qemu_driver: add support to perf event perf: add new xml element perf: reenable perf events when libvirtd restart virsh: implement new command to support perf virsh: extend domstats command daemon/remote.c | 47 docs/schemas/domaincommon.rng | 27 +++ include/libvirt/libvirt-domain.h | 19 ++ include/libvirt/virterror.h | 1 + src/Makefile.am | 1 + src/conf/domain_conf.c| 111 ++ src/conf/domain_conf.h| 10 + src/driver-hypervisor.h | 12 + src/libvirt-domain.c | 93 src/libvirt_private.syms | 12 + src/libvirt_public.syms | 6 + src/qemu/qemu_domain.h| 3 + src/qemu/qemu_driver.c| 195 + src/qemu/qemu_process.c | 10 + src/remote/remote_driver.c| 39 src/remote/remote_protocol.x | 30 ++- src/remote_protocol-structs | 18 ++ src/util/virerror.c | 1 + src/util/virperf.c| 303 ++ src/util/virperf.h| 63 ++ tests/domainschemadata/domain-perf-simple.xml | 20 ++ tools/virsh-domain-monitor.c | 7 + tools/virsh-domain.c | 128 +++ tools/virsh.pod | 27 ++- 24 files changed, 1180 insertions(+), 3 deletions(-) create mode 100644 src/util/virperf.c create mode 100644 src/util/virperf.h create mode 100644 tests/domainschemadata/domain-perf-simple.xml -- 1.9.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] qemu: Use qemuDomainAdjustMaxMemLock()
On 12/10/2015 05:43 AM, Andrea Bolognani wrote: > On Wed, 2015-12-09 at 15:15 -0500, John Ferlan wrote: >> On 11/24/2015 08:56 AM, Andrea Bolognani wrote: >>> Replace all uses of the qemuDomainRequiresMlock/virProcessSetMaxMemLock >>> combination with the equivalent qemuDomainAdjustMaxMemLock() call. >>> --- >>> src/qemu/qemu_hotplug.c | 40 +--- >>> 1 file changed, 13 insertions(+), 27 deletions(-) >> >> OK - so now I see how this is going to be used... patch 3's multi >> purpose MaxMemLock makes a bit more sense; however, I'm still wondering >> how we'd ever get to a point where the a last removal wouldn't have set >> the lockbytes value to anything but the original (yes, patch 5 is >> missing from the current removal)... >> >> Seems like perhaps the original_memlock is fixing the bug that was shown >> in patch 5 - that the hostdev removal didn't return our value properly. >> >> IOW: If the original_memlock adjustment was taken out, is there a case >> (after patch 5 is applied) where when memory locking is no longer >> required that the value after the removal wouldn't have been what was >> saved in original_memlock. > > Hi John, > > first of all, thanks for the review. I'm in the process of going through > all your comments, but I wanted to address this one first. > > I tried to give a somewhat detailed overview of the series in the cover > letter to help reviewers understand how the single commits fit > toghether, but it's always hard to balance the amount of information > when you've just finished implementing something and all the details are > still so completely clear in your head. Any tips on how the overview > could have been more helpful are appreciated! I always appreciate extra comments and cover letters, but they are a delicate balance. It's hard when you're so embroiled in your patches to step back and take that 10,000 foot view. There's no secret sauce for that. Yes I read cover letters, but then I start reviewing patches and that cover letter can easily get paged out of short term memory. When I review I try to take patches one at a time, but I will peek ahead as well when I get curious about the why something is being done. I'll also try to provide enough context in reviews to try an "show" what I was thinking while reviewing. Seeing terse reviews of "this is wrong" or "I wouldn't do it that way" in my mind are not very helpful. > > The whole issue stems from the use of this kind of construct: > > if (qemuDomainRequiresMlock(vm->def)) > virProcessSetMaxMemLock(vm->pid, > qemuDomainGetMlockLimitBytes(vm->def)); > > qemuDomainRequiresMlock() is always true on ppc64, but on x86 it's true > only if at least one VFIO device is currently assigned to the guest, > which means it can return false both because no VFIO device has been > assigned to the guest yet or because we just removed the last one. As I got to patch 4 & 5 I think it started to sink in again that the issue was more because of x86. Once I read and thought about patch 5, then I went back to my patch 4 comments and added the extra wording around whether or not using original_memlock would be useful and of course to separate out "code motion" from "new code". Of course by that time patch 3 was already sent so I couldn't go "adjust" my comments there. > > In the first case, we don't need to adjust the memory lock limit and > everything is peachy; in the latter case, though, we *had* increased the > memory locking limit when we assigned the first VFIO device and we > should lower it now that's no longer required, but because of the code > above we don't. > > qemuDomainAdjustMaxMemLock() solves this by storing the previous memory > locking limit when first increasing it, so now we can tell whether > memory locking is simply not needed or *no longer* needed, and we can > behave accordingly - not doing anything in the former case, restoring > the previous value in the latter. > >> Perhaps would have been easier to have added the Adjust code without >> original_memlock, then replace code as is done here, then add the >> hostdev case, then add the original value. Just trying to separate new >> functionality from code motion/creation of helper code. > > It's never easy to order changes, is it? :) Well better than what my former job had - here's a steaming pile of changes with an explanation of the changes. Sometimes you got a good explanation of what the changes did, while other times it was "this fixes bug #N". Um, gee thanks! John > > I can see how using the ordering you propose would make it easier to > follow along, though. I will shuffle commits according to your > suggestion for v2. > > Cheers. > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vz: Allow to create container based on template
24.11.2015 17:14, Dmitry Guryanov пишет:
On Thu, 2015-11-19 at 14:44 +0300, Mikhail Feoktistov wrote:
We shouldn't delete disk from default config if we create container based on
template,
because we don't have the new disk from XML, only template name.
And don't add template section from XML as new filesystem,
we use PrlVmCfg_SetOsTemplate function to set template name.
---
src/vz/vz_sdk.c | 33 +
1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 89c9e89..865cabe 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2096,12 +2096,14 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom,
virDomainDefPtr def)
return 0;
}
-static int prlsdkClearDevices(PRL_HANDLE sdkdom)
+static int prlsdkClearDevices(PRL_HANDLE sdkdom, bool skipdisk)
{
PRL_RESULT pret;
PRL_UINT32 n, i;
PRL_HANDLE devList;
PRL_HANDLE dev;
+PRL_DEVICE_TYPE devType;
+PRL_VM_DEV_EMULATION_TYPE emul;
int ret = -1;
pret = PrlVmCfg_SetVNCMode(sdkdom, PRD_DISABLED);
@@ -2117,6 +2119,18 @@ static int prlsdkClearDevices(PRL_HANDLE sdkdom)
pret = PrlHndlList_GetItem(devList, i, &dev);
prlsdkCheckRetGoto(pret, cleanup);
+if (skipdisk) {
+pret = PrlVmDev_GetType(dev, &devType);
+prlsdkCheckRetGoto(pret, cleanup);
+
+pret = PrlVmDev_GetEmulatedType(dev, &emul);
Where do you use this emul?
This emul is unusable variable. Thanks.
+prlsdkCheckRetGoto(pret, cleanup);
+
+if (devType == PDE_HARD_DISK) {
+PrlHandle_Free(dev);
+continue;
+}
+}
pret = PrlVmDev_Remove(dev);
PrlHandle_Free(dev);
}
@@ -3465,6 +3479,7 @@ prlsdkDoApplyConfig(virConnectPtr conn,
char uuidstr[VIR_UUID_STRING_BUFLEN + 2];
bool needBoot = true;
char *mask = NULL;
+bool skipdisk = false;
if (prlsdkCheckUnsupportedParams(sdkdom, def) < 0)
return -1;
@@ -3514,7 +3529,11 @@ prlsdkDoApplyConfig(virConnectPtr conn,
}
prlsdkCheckRetGoto(pret, error);
-if (prlsdkClearDevices(sdkdom) < 0)
+if (def->nfss == 1 &&
+def->fss[0]->type == VIR_DOMAIN_FS_TYPE_TEMPLATE)
+skipdisk = true;
+
+if (prlsdkClearDevices(sdkdom, skipdisk) < 0)
goto error;
I think we should make this logic more robust. There is only one case,
when VIR_DOMAIN_FS_TYPE_TEMPLATE fs is allowed - when we create new container.
So I'd add a parameter to prlsdkDoApplyConfig, something like useCtTemplateFs,
which means that we should have only one fs of type template and no disks.
Ok. done in patch v2.
if (prlsdkRemoveBootDevices(sdkdom) < 0)
@@ -3544,6 +3563,8 @@ prlsdkDoApplyConfig(virConnectPtr conn,
for (i = 0; i < def->nfss; i++) {
if (STREQ(def->fss[i]->dst, "/"))
needBoot = false;
+if (def->fss[i]->type == VIR_DOMAIN_FS_TYPE_TEMPLATE)
+continue;
If fs with index different from 0 is VIR_DOMAIN_FS_TYPE_TEMPLATE - it's an
error, also if we are not creating new ct - it's also error.
done in patch v2.
if (prlsdkAddFS(sdkdom, def->fss[i]) < 0)
goto error;
}
@@ -3655,6 +3676,7 @@ prlsdkCreateCt(virConnectPtr conn, virDomainDefPtr def)
int ret = -1;
int useTemplate = 0;
size_t i;
+PRL_UINT32 flags = 0;
if (def->nfss > 1) {
/* Check all filesystems */
@@ -3696,8 +3718,11 @@ prlsdkCreateCt(virConnectPtr conn, virDomainDefPtr def)
if (ret)
goto cleanup;
-job = PrlVm_RegEx(sdkdom, "",
- PACF_NON_INTERACTIVE_MODE | PRNVM_PRESERVE_DISK);
+flags = PACF_NON_INTERACTIVE_MODE;
+if (!useTemplate)
+flags = flags | PRNVM_PRESERVE_DISK;
Why do you need to remove this flag to create ct from template? As I remember
it's needed to keep disk images, which you've remove from config.
If we create ct from template with flag PRNVM_PRESERVE_DISK than error
occurs
err=SDabbot stat /vz/root/22eba3c0-05d7-452f-89df-5551ec2c85a1: No such
file or directory Failed to start the Container
So we should not set this flag se of create from template.
+
+job = PrlVm_RegEx(sdkdom, "", flags);
if (PRL_FAILED(waitJob(job)))
ret = -1;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] qemu: Use qemuDomainAdjustMaxMemLock()
On Wed, 2015-12-09 at 15:15 -0500, John Ferlan wrote: > On 11/24/2015 08:56 AM, Andrea Bolognani wrote: > > Replace all uses of the qemuDomainRequiresMlock/virProcessSetMaxMemLock > > combination with the equivalent qemuDomainAdjustMaxMemLock() call. > > --- > > src/qemu/qemu_hotplug.c | 40 +--- > > 1 file changed, 13 insertions(+), 27 deletions(-) > > OK - so now I see how this is going to be used... patch 3's multi > purpose MaxMemLock makes a bit more sense; however, I'm still wondering > how we'd ever get to a point where the a last removal wouldn't have set > the lockbytes value to anything but the original (yes, patch 5 is > missing from the current removal)... > > Seems like perhaps the original_memlock is fixing the bug that was shown > in patch 5 - that the hostdev removal didn't return our value properly. > > IOW: If the original_memlock adjustment was taken out, is there a case > (after patch 5 is applied) where when memory locking is no longer > required that the value after the removal wouldn't have been what was > saved in original_memlock. Hi John, first of all, thanks for the review. I'm in the process of going through all your comments, but I wanted to address this one first. I tried to give a somewhat detailed overview of the series in the cover letter to help reviewers understand how the single commits fit toghether, but it's always hard to balance the amount of information when you've just finished implementing something and all the details are still so completely clear in your head. Any tips on how the overview could have been more helpful are appreciated! The whole issue stems from the use of this kind of construct: if (qemuDomainRequiresMlock(vm->def)) virProcessSetMaxMemLock(vm->pid, qemuDomainGetMlockLimitBytes(vm->def)); qemuDomainRequiresMlock() is always true on ppc64, but on x86 it's true only if at least one VFIO device is currently assigned to the guest, which means it can return false both because no VFIO device has been assigned to the guest yet or because we just removed the last one. In the first case, we don't need to adjust the memory lock limit and everything is peachy; in the latter case, though, we *had* increased the memory locking limit when we assigned the first VFIO device and we should lower it now that's no longer required, but because of the code above we don't. qemuDomainAdjustMaxMemLock() solves this by storing the previous memory locking limit when first increasing it, so now we can tell whether memory locking is simply not needed or *no longer* needed, and we can behave accordingly - not doing anything in the former case, restoring the previous value in the latter. > Perhaps would have been easier to have added the Adjust code without > original_memlock, then replace code as is done here, then add the > hostdev case, then add the original value. Just trying to separate new > functionality from code motion/creation of helper code. It's never easy to order changes, is it? :) I can see how using the ordering you propose would make it easier to follow along, though. I will shuffle commits according to your suggestion for v2. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] VFIO fixes for PCI devices
On Wed, 2015-12-09 at 08:09 -0700, Alex Williamson wrote: > > I was under the impression that what the current series > > does, eg. sharing devices in the same IOMMU group between > > the host driver and vfio-pci is safe as long as no guest is > > using them at the same time, and that devices could be > > safely "moved" between the host driver (eg. in use) and > > vfio-pci (eg. idle, waiting to be assigned to a guest) as > > many times as desired without ill consequences. > > > > Is my understanding wrong? Do I need to rework the series > > so that unbinds and reprobes are always executed across the > > IOMMU group? > > Hi Andrea, > > Your understanding is correct, so I think it just comes down to how sure > you are that the vfio group is idle/unused. If there's any risk that a > device is still in use by QEMU, then we haven't solved the original > problem. Unbinding isn't the only way to have good confidence of this > though. You could track the QEMU pid, you could use fuser to make sure > the group is not in use, you could try to open the group yourself to > make sure it's not in use, and of course you can unbind as proposed in > the second option. So long as you know the group is idle, somehow, it > shouldn't matter what order you unbind and reprobe. Thanks for the confirmation, Alex. What we're doing right now in libvirt (and my series extends on that) is keeping track of each device's state internally by recording information such as which guest, if any, is using it. We're mostly assuming that the user will not mess with the configuration behind our back. I guess adding more checks to make sure that's actually the case would be good, but then again, we can hardly stop the user from writing stuff into /sys :) At least now we know my series is not built upon a completely incorrect understanding of the constraints! Quick follow-up question: all of this care with IOMMU group ownership is only really applied when dealing with VFIO passthrough - are legacy KVM assignment and Xen assignment really not affected at all? Is the IOMMU not involved in the process? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
