[libvirt] Fwd: [PATCH v2] Close the source fd if the destination qemu exits during tunnelled migration
On Tue, Sep 29, 2015 at 2:33 AM, John Ferlan wrote: > > > On 09/28/2015 11:02 AM, Shivaprasad bhat wrote: >> Hi Jon, >> >> Thanks a lot for attaching the patch. Replies inline. >> >> On Thu, Sep 24, 2015 at 1:52 AM, John Ferlan wrote: >>> >>> >>> On 09/22/2015 07:21 AM, Shivaprasad bhat wrote: On Mon, Sep 21, 2015 at 8:04 PM, John Ferlan wrote: > > > On 09/21/2015 05:09 AM, Shivaprasad bhat wrote: >> Thanks John for the comments. >> >> >> On Fri, Sep 18, 2015 at 10:34 PM, John Ferlan wrote: >>> >>> >>> On 09/14/2015 10:44 AM, Shivaprasad G Bhat wrote: Tunnelled migration can hang if the destination qemu exits despite all the ABI checks. This happens whenever the destination qemu exits before the complete transfer is noticed by source qemu. The savevm state checks at runtime can fail at destination and cause qemu to error out. The source qemu cant notice it as the EPIPE is not propogated to it. The qemuMigrationIOFunc() notices the stream being broken from virStreamSend() and it cleans up the stream alone. The qemuMigrationWaitForCompletion() would never get to 100% transfer completion. The qemuMigrationWaitForCompletion() never breaks out as well since the ssh connection to destination is healthy, and the source qemu also thinks the migration is ongoing as the Fd to which it transfers, is never closed or broken. So, the migration will hang forever. Even Ctrl-C on the virsh migrate wouldn't be honoured. Close the source side FD when there is an error in the stream. That way, the source qemu updates itself and qemuMigrationWaitForCompletion() notices the failure. Close the FD for all kinds of errors to be sure. The error message is not copied for EPIPE so that the destination error is copied instead later. Note: Reproducible with repeated migrations between Power hosts running in different subcores-per-core modes. Changes from v1 -> v2: VIR_FORCE_CLOSE() was called twice for this use case which would log unneccessary warnings. So, move the fd close to qemuMigrationIOFunc so that there are no unnecessary duplicate attempts.(Would this trigger a Coverity error? I don't have a setup to check.) Signed-off-by: Shivaprasad G Bhat --- src/qemu/qemu_migration.c |8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index ff89ab5..9602fb2 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4012,6 +4012,7 @@ static void qemuMigrationIOFunc(void *arg) if (virStreamFinish(data->st) < 0) goto error; +VIR_FORCE_CLOSE(data->sock); VIR_FREE(buffer); return; @@ -4029,7 +4030,11 @@ static void qemuMigrationIOFunc(void *arg) } error: -virCopyLastError(&data->err); +/* Let the source qemu know that the transfer cant continue anymore. + * Don't copy the error for EPIPE as destination has the actual error. */ +VIR_FORCE_CLOSE(data->sock); +if (!virLastErrorIsSystemErrno(EPIPE)) +virCopyLastError(&data->err); virResetLastError(); VIR_FREE(buffer); } @@ -4397,7 +4402,6 @@ qemuMigrationRun(virQEMUDriverPtr driver, if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0) ret = -1; } -VIR_FORCE_CLOSE(fd); >>> >>> ^^^ >>> >>> This causes Coverity to claim a RESOURCE_LEAK >>> >>> Feels like this was a mistake edit... >>> >> >> The VIR_FORCE_CLOSE() inside qemuMigrationIOFunc() alone is sufficient. >> Having this again here would lead to Warning in the logs. I too thought >> coverity >> would complain. Is there a way to force ignore a coverity warning? >> > > Typically a marker of sorts, such as > > /* coverity[leaked_handle] */ > > Although I'm not sure that's the best way to handle this either. > > The problem I see though is this is an "error path" issue and when > perhaps it's safe/required to close fd/io->sock/data->sock. > > Your commit comment notes that the issue is seen on a fairly specific > event (virStreamSend failure) for a specific type of migration. I believe the failure can be seen for all types of migration with savestate mismatch. >>> >>> My t
Re: [libvirt] [PATCH 3/3] testutils: Drop virtTestResult
On Tue, Sep 29, 2015 at 07:56:47PM -0400, Cole Robinson wrote: virtTestResult is suboptimal for a few reasons: - It poorly duplicates virtTestRun pass/fail reporting logic - It doesn't have virtTestRun's alloc testing support - It only reports the test name _after_ the test has run. - It doesn't follow the standard virtTestRun pattern that most other tests use. There's no users left, so drop it. If any other async tests like eventtest spring up that don't cleanly fit the virtTestRun pattern, I suggest they just open code the support for it around virtTestRun --- ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] testutils: Add coloring to verbose PASS/FAILED output
On Tue, Sep 29, 2015 at 07:54:50PM -0400, Cole Robinson wrote: Helps to visually track down test failures if debugging the test suite. The colors match what 'make check' does for pass/fail/skip --- Consistency is always fine, ACK. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] tests: sheepdog: Drop use of virtTestResult
On Tue, Sep 29, 2015 at 07:56:45PM -0400, Cole Robinson wrote: Instead use the same pattern that most other test files use. --- tests/storagebackendsheepdogtest.c | 63 -- 1 file changed, 47 insertions(+), 16 deletions(-) ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] tests: eventtest: Open code virtTestResult
On Tue, Sep 29, 2015 at 07:56:46PM -0400, Cole Robinson wrote:
These event tests aren't run synchronously, so there isn't an obvious
function to pass to virtTestRun. Instead, open code roughly what
virtTestResult did before: printing an error message if a test failed.
---
tests/eventtest.c | 57 +--
1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/tests/eventtest.c b/tests/eventtest.c
index 13adbf6..ab08181 100644
--- a/tests/eventtest.c
+++ b/tests/eventtest.c
@@ -63,6 +63,43 @@ enum {
EV_ERROR_DATA,
};
+struct testEventResultData {
+bool failed;
+const char *msg;
+};
+
+static int
+testEventResultCallback(const void *opaque)
+{
+const struct testEventResultData *data = opaque;
+
+if (data->failed && data->msg) {
+fprintf(stderr, "%s", data->msg);
+}
+return data->failed;
+}
+
+static void
+ATTRIBUTE_FMT_PRINTF(3,4)
+testEventReport(const char *name, bool failed, const char *msg, ...)
+{
+va_list vargs;
+va_start(vargs, msg);
+char *str = NULL;
+struct testEventResultData data;
+
+if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) {
+failed = true;
+}
+
+data.failed = failed;
+data.msg = msg;
I think you meant data.msg = str; here.
ACK with that changed and your amendment squashed in.
P.S.: If you'd really like, I think this test could be made to use
virTestRun as well ;)
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainAttachDeviceLive: Check provided disk address
On Tue, Sep 29, 2015 at 05:27:58PM -0400, John Ferlan wrote:
On 09/29/2015 11:03 AM, Michal Privoznik wrote:
On 25.09.2015 14:45, Martin Kletzander wrote:
On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote:
On 09/25/2015 05:38 AM, Michal Privoznik wrote:
On 25.09.2015 11:36, Martin Kletzander wrote:
On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Imagine an user who is trying to attach a disk to a domain with
the following XML:
The XML is obviously wrong. It's trying to attach a virtio disk
onto non-PCI bus. We should forbid that.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_hotplug.c | 7 +++
1 file changed, 7 insertions(+)
How come this is not handled in qemuDomainAssignAddresses(), it
doesn't get called? There's a check for exactly that in
qemuAssignDevicePCISlots().
Exactly! qemuAssignDevicePCISlots() is called only in case of --config.
Seems to me this may be more of a generic problem - a user providing the
wrong address type for the type of device. I have a recollection of
Yes, and since we have checks for those, it's confusing to me why
would qemuAssignDevicePCISlots() be called only with --config. Can we
use that code which checks for more things already? For example, the
here-missing virtio-mmio.
Yes and no. qemuDomainAssignAddresses() expects to see vm->def which
already has the device attached. Moreover, it not only works over all
devices in the domain (filling in all the missing addresses), but
possibly creating new controllers too (e.g. plugging new pci bridges if
running out of addresses on a bus).
Does anybody have a bright idea how this can be fixed apart from obvious
one - breaking whole address assignment code into parts and reassembling
it back together again?
From the v4 from Ruifeng Beng:
http://www.redhat.com/archives/libvir-list/2015-September/msg00524.html
which has some validity w/r/t using qemuCheckDiskConfig; however, I'm
wondering now if that's far too late in processing. Is it agreeable that
the checks in virDomainDeviceAddressTypeIsValid will cover the cases
when the incoming device has an address in the XML?
Why not use the power of virDomainDeviceDefPostParseInternal? There's a
check in that code:
if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0)
return -1;
why not make an "else" which says, let's be sure the disk->info.type
provided is valid?
I've attached a patch which works for the cases shown in the bz (both no
params and --config).
This would solve attach, update, and detach since each would call the
virDomainDeviceDefParse which calls the PostParse code.
John
NOTE: The change to the test is because the failure now occurs during
parse rather than at run (e.g. earlier, where I think it should).
I agree, and this sounds good, I'd have just two minor additions, see
below.
From a03a8aa073eb410d6b713e6f77572edcf0631263 Mon Sep 17 00:00:00 2001
From: John Ferlan
Date: Tue, 29 Sep 2015 17:04:11 -0400
Subject: [PATCH] bug 1257844
Signed-off-by: John Ferlan
---
src/conf/domain_conf.c | 43 +++
tests/qemuxml2argvtest.c | 2 +-
2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 393ece7..21439c7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3962,6 +3962,41 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
}
+/* Check if a provided address is valid */
+static bool
+virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
1) The name suggests it checks the address type of any device, I'd
somehow add a "Disk" in the name =)
2) Until anything similar to my proposal [1] is added, this would
make daemon lose such domain on reload.
Martin
[1] https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
+{
+if (disk->info.type) {
+switch (disk->bus) {
+case VIR_DOMAIN_DISK_BUS_IDE:
+case VIR_DOMAIN_DISK_BUS_SCSI:
+case VIR_DOMAIN_DISK_BUS_SATA:
+case VIR_DOMAIN_DISK_BUS_FDC:
+if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
+return false;
+break;
+case VIR_DOMAIN_DISK_BUS_USB:
+if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
+return false;
+break;
+case VIR_DOMAIN_DISK_BUS_VIRTIO:
+if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
+disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390
&&
+disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO
&&
+disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+return false;
+break;
+case VIR_DOMAIN_DISK_BUS_XEN:
+case VIR_DOMAIN_DISK_BUS_UML:
+
[libvirt] [PATCH 2/2] util: Produce friendlier error message to user
Commit 1c24cfe9 fix the problem in virNumaSetPagePoolSize,
this patch just like it and fix the issue in another function.
when user use freepages and specify a invalid node or page size,
will get error like this:
# virsh freepages 0 1
error: Failed to open file
'/sys/devices/system/node/node0/hugepages/hugepages-1kB/free_hugepages': No
such file or directory
Add two checks to catch this and therefore produce much more
friendlier error messages.
Signed-off-by: Luyao Huang
---
src/util/virnuma.c | 12
1 file changed, 12 insertions(+)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 8577bd8..c8beade 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -560,6 +560,12 @@ virNumaGetHugePageInfo(int node,
page_size, "nr_hugepages") < 0)
goto cleanup;
+if (!virFileExists(path)) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("page size or NUMA node not available"));
+goto cleanup;
+}
+
if (virFileReadAll(path, 1024, &buf) < 0)
goto cleanup;
@@ -579,6 +585,12 @@ virNumaGetHugePageInfo(int node,
page_size, "free_hugepages") < 0)
goto cleanup;
+if (!virFileExists(path)) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("page size or NUMA node not available"));
+goto cleanup;
+}
+
if (virFileReadAll(path, 1024, &buf) < 0)
goto cleanup;
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] improve the error in 2 function
*** BLURB HERE *** Luyao Huang (2): util: split the virNumaGetHugePageInfoPath into separate function util: Produce friendlier error message to user src/util/virnuma.c | 54 ++ 1 file changed, 30 insertions(+), 24 deletions(-) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] util: split the virNumaGetHugePageInfoPath into separate function
https://bugzilla.redhat.com/show_bug.cgi?id=1265114
When pass 0 page_size to virNumaGetHugePageInfoPath function, we will
get fail like this:
error : virFileReadAll:1358 : Failed to read file
'/sys/devices/system/node/node0/hugepages/': Is a directory
Because when the page_size is 0 the virNumaGetHugePageInfoPath will
build the directory of system path, but we don't want that.
Introduce a new helper to build the dir path could avoid this issue.
Signed-off-by: Luyao Huang
---
src/util/virnuma.c | 42 ++
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 1a62d62..8577bd8 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -493,45 +493,39 @@ virNumaGetHugePageInfoPath(char **path,
unsigned int page_size,
const char *suffix)
{
-
-int ret = -1;
-
if (node == -1) {
/* We are aiming at overall system info */
-if (page_size) {
-/* And even on specific huge page size */
if (virAsprintf(path,
HUGEPAGES_SYSTEM_PREFIX HUGEPAGES_PREFIX "%ukB/%s",
page_size, suffix ? suffix : "") < 0)
-goto cleanup;
-} else {
-if (VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX) < 0)
-goto cleanup;
-}
-
+return -1;
} else {
/* We are aiming on specific NUMA node */
-if (page_size) {
-/* And even on specific huge page size */
if (virAsprintf(path,
HUGEPAGES_NUMA_PREFIX "node%d/hugepages/"
HUGEPAGES_PREFIX "%ukB/%s",
node, page_size, suffix ? suffix : "") < 0)
-goto cleanup;
-} else {
-if (virAsprintf(path,
-HUGEPAGES_NUMA_PREFIX "node%d/hugepages/",
-node) < 0)
-goto cleanup;
-}
+return -1;
}
-ret = 0;
- cleanup:
-return ret;
+return 0;
}
+static int
+virNumaGetHugePageInfoDir(char **path, int node)
+{
+if (node == -1) {
+if (VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX) < 0)
+return -1;
+} else {
+if (virAsprintf(path,
+HUGEPAGES_NUMA_PREFIX "node%d/hugepages/",
+node) < 0)
+return -1;
+}
+return 0;
+}
/**
* virNumaGetHugePageInfo:
* @node: NUMA node id
@@ -724,7 +718,7 @@ virNumaGetPages(int node,
* is always shown as used memory. Here, however, we want to report
* slightly different information. So we take the total memory on a node
* and subtract memory taken by the huge pages. */
-if (virNumaGetHugePageInfoPath(&path, node, 0, NULL) < 0)
+if (virNumaGetHugePageInfoDir(&path, node) < 0)
goto cleanup;
if (!(dir = opendir(path))) {
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] kvm-all: notice KVM of vcpu's TSC rate after migration
On Tue, Sep 29, 2015 at 03:02:07PM -0300, Eduardo Habkost wrote:
> On Tue, Sep 29, 2015 at 11:43:34AM +0800, Haozhong Zhang wrote:
> > On Mon, Sep 28, 2015 at 01:37:34PM -0300, Eduardo Habkost wrote:
> > > On Mon, Sep 28, 2015 at 01:38:31PM +0800, Haozhong Zhang wrote:
> [...]
> > > > static void do_kvm_cpu_synchronize_post_init(void *arg)
> > > > {
> > > > CPUState *cpu = arg;
> > > > +CPUX86State *env = &X86_CPU(cpu)->env;
> > > > +int r;
> > > > +
> > > > +/*
> > > > + * XXX: KVM_SET_TSC_KHZ must be done before
> > > > kvm_arch_put_registers().
> > >
> > > Could you explain where this requirement comes from?
> > >
> >
> > kvm_arch_put_registers() below will setup vcpu's MSR_IA32_TSC through
> > KVM ioctl KVM_SET_MSRS. KVM needs to know vcpu's TSC rate so as to
> > correctly scale the TSC value given by QEMU, especially when vcpu's
> > TSC rate is different than the host TSC rate (e.g. it's migrated from
> > another machine w/ different host TSC rate than the current one).
>
> Thanks. The comment above could contain a short version of this
> explanation, e.g.: "KVM needs KVM_SET_TSC_KHZ to be done before
> KVM_SET_MSRS sets MSR_IA32_TSC (done by kvm_arch_put_registers())".
>
will include this in the comment
> >
> [...]
> > > Or maybe we shouldn't treat this as VM state, but as configuration, and
> > > let management configure the TSC frequency explicitly if the user really
> > > needs it to stay the same during migration.
> > >
> > > (CCing libvir-list to see if they have feedback)
> > >
> >
> > Thanks for CC. I'll consider to add a command line option to control
> > the configuration of guest TSC fequency.
>
> It already exists, -cpu has a "tsc-freq" option.
>
What I'm considering is to add a "-keep-tsc-freq" option to control
whether the TSC frequency should be migrated if "tsc-freq" is not
presented. Compared to "tsc-freq", "-keep-tsc-freq" can free users
from figuring out the guest TSC frequency manually in the migration.
- Haozhong
> --
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] tests: eventtest: Open code virtTestResult
On 09/29/2015 07:56 PM, Cole Robinson wrote:
> These event tests aren't run synchronously, so there isn't an obvious
> function to pass to virtTestRun. Instead, open code roughly what
> virtTestResult did before: printing an error message if a test failed.
> ---
> tests/eventtest.c | 57
> +--
> 1 file changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/tests/eventtest.c b/tests/eventtest.c
> index 13adbf6..ab08181 100644
> --- a/tests/eventtest.c
> +++ b/tests/eventtest.c
> @@ -63,6 +63,43 @@ enum {
> EV_ERROR_DATA,
> };
>
> +struct testEventResultData {
> +bool failed;
> +const char *msg;
> +};
> +
> +static int
> +testEventResultCallback(const void *opaque)
> +{
> +const struct testEventResultData *data = opaque;
> +
> +if (data->failed && data->msg) {
> +fprintf(stderr, "%s", data->msg);
> +}
> +return data->failed;
> +}
> +
> +static void
> +ATTRIBUTE_FMT_PRINTF(3,4)
> +testEventReport(const char *name, bool failed, const char *msg, ...)
> +{
> +va_list vargs;
> +va_start(vargs, msg);
> +char *str = NULL;
> +struct testEventResultData data;
> +
> +if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) {
> +failed = true;
> +}
> +
> +data.failed = failed;
> +data.msg = msg;
> +virtTestRun(name, testEventResultCallback, &data);
> +
> +va_end(vargs);
> +VIR_FREE(str);
> +}
> +
> static void
> testPipeReader(int watch, int fd, int events, void *data)
> {
> @@ -148,13 +185,13 @@ verifyFired(const char *name, int handle, int timer)
> for (i = 0; i < NUM_FDS; i++) {
> if (handles[i].fired) {
> if (i != handle) {
> -virtTestResult(name, 1,
> +testEventReport(name, 1,
> "Handle %zu fired, but expected %d\n", i,
> handle);
> return EXIT_FAILURE;
> } else {
> if (handles[i].error != EV_ERROR_NONE) {
> -virtTestResult(name, 1,
> +testEventReport(name, 1,
> "Handle %zu fired, but had error %d\n", i,
> handles[i].error);
> return EXIT_FAILURE;
> @@ -163,7 +200,7 @@ verifyFired(const char *name, int handle, int timer)
> }
> } else {
> if (i == handle) {
> -virtTestResult(name, 1,
> +testEventReport(name, 1,
> "Handle %d should have fired, but didn't\n",
> handle);
> return EXIT_FAILURE;
> @@ -171,7 +208,7 @@ verifyFired(const char *name, int handle, int timer)
> }
> }
> if (handleFired != 1 && handle != -1) {
> -virtTestResult(name, 1,
> +testEventReport(name, 1,
> "Something weird happened, expecting handle %d\n",
> handle);
> return EXIT_FAILURE;
> @@ -181,12 +218,12 @@ verifyFired(const char *name, int handle, int timer)
> for (i = 0; i < NUM_TIME; i++) {
> if (timers[i].fired) {
> if (i != timer) {
> -virtTestResult(name, 1,
> +testEventReport(name, 1,
> "Timer %zu fired, but expected %d\n", i,
> timer);
> return EXIT_FAILURE;
> } else {
> if (timers[i].error != EV_ERROR_NONE) {
> -virtTestResult(name, 1,
> +testEventReport(name, 1,
> "Timer %zu fired, but had error %d\n", i,
> timers[i].error);
> return EXIT_FAILURE;
> @@ -195,7 +232,7 @@ verifyFired(const char *name, int handle, int timer)
> }
> } else {
> if (i == timer) {
> -virtTestResult(name, 1,
> +testEventReport(name, 1,
> "Timer %d should have fired, but didn't\n",
> timer);
> return EXIT_FAILURE;
> @@ -203,7 +240,7 @@ verifyFired(const char *name, int handle, int timer)
> }
> }
> if (timerFired != 1 && timer != -1) {
> -virtTestResult(name, 1,
> +testEventReport(name, 1,
> "Something weird happened, expecting timer %d\n",
> timer);
> return EXIT_FAILURE;
> @@ -234,14 +271,14 @@ finishJob(const char *name, int handle, int timer)
> rc = pthread_cond_timedwait(&eventThreadJobCond, &eventThreadMutex,
> &waitTime);
> if (rc != 0) {
> -virtTestResult(name, 1, "Timed out waiting for pipe event\n");
> +testEventReport(name, 1, "Timed out waiting for pipe event\n");
> return EXIT_FAILURE;
> }
[libvirt] [PATCH 2/3] tests: eventtest: Open code virtTestResult
These event tests aren't run synchronously, so there isn't an obvious
function to pass to virtTestRun. Instead, open code roughly what
virtTestResult did before: printing an error message if a test failed.
---
tests/eventtest.c | 57 +--
1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/tests/eventtest.c b/tests/eventtest.c
index 13adbf6..ab08181 100644
--- a/tests/eventtest.c
+++ b/tests/eventtest.c
@@ -63,6 +63,43 @@ enum {
EV_ERROR_DATA,
};
+struct testEventResultData {
+bool failed;
+const char *msg;
+};
+
+static int
+testEventResultCallback(const void *opaque)
+{
+const struct testEventResultData *data = opaque;
+
+if (data->failed && data->msg) {
+fprintf(stderr, "%s", data->msg);
+}
+return data->failed;
+}
+
+static void
+ATTRIBUTE_FMT_PRINTF(3,4)
+testEventReport(const char *name, bool failed, const char *msg, ...)
+{
+va_list vargs;
+va_start(vargs, msg);
+char *str = NULL;
+struct testEventResultData data;
+
+if (msg && virVasprintfQuiet(&str, msg, vargs) != 0) {
+failed = true;
+}
+
+data.failed = failed;
+data.msg = msg;
+virtTestRun(name, testEventResultCallback, &data);
+
+va_end(vargs);
+VIR_FREE(str);
+}
+
static void
testPipeReader(int watch, int fd, int events, void *data)
{
@@ -148,13 +185,13 @@ verifyFired(const char *name, int handle, int timer)
for (i = 0; i < NUM_FDS; i++) {
if (handles[i].fired) {
if (i != handle) {
-virtTestResult(name, 1,
+testEventReport(name, 1,
"Handle %zu fired, but expected %d\n", i,
handle);
return EXIT_FAILURE;
} else {
if (handles[i].error != EV_ERROR_NONE) {
-virtTestResult(name, 1,
+testEventReport(name, 1,
"Handle %zu fired, but had error %d\n", i,
handles[i].error);
return EXIT_FAILURE;
@@ -163,7 +200,7 @@ verifyFired(const char *name, int handle, int timer)
}
} else {
if (i == handle) {
-virtTestResult(name, 1,
+testEventReport(name, 1,
"Handle %d should have fired, but didn't\n",
handle);
return EXIT_FAILURE;
@@ -171,7 +208,7 @@ verifyFired(const char *name, int handle, int timer)
}
}
if (handleFired != 1 && handle != -1) {
-virtTestResult(name, 1,
+testEventReport(name, 1,
"Something weird happened, expecting handle %d\n",
handle);
return EXIT_FAILURE;
@@ -181,12 +218,12 @@ verifyFired(const char *name, int handle, int timer)
for (i = 0; i < NUM_TIME; i++) {
if (timers[i].fired) {
if (i != timer) {
-virtTestResult(name, 1,
+testEventReport(name, 1,
"Timer %zu fired, but expected %d\n", i, timer);
return EXIT_FAILURE;
} else {
if (timers[i].error != EV_ERROR_NONE) {
-virtTestResult(name, 1,
+testEventReport(name, 1,
"Timer %zu fired, but had error %d\n", i,
timers[i].error);
return EXIT_FAILURE;
@@ -195,7 +232,7 @@ verifyFired(const char *name, int handle, int timer)
}
} else {
if (i == timer) {
-virtTestResult(name, 1,
+testEventReport(name, 1,
"Timer %d should have fired, but didn't\n",
timer);
return EXIT_FAILURE;
@@ -203,7 +240,7 @@ verifyFired(const char *name, int handle, int timer)
}
}
if (timerFired != 1 && timer != -1) {
-virtTestResult(name, 1,
+testEventReport(name, 1,
"Something weird happened, expecting timer %d\n",
timer);
return EXIT_FAILURE;
@@ -234,14 +271,14 @@ finishJob(const char *name, int handle, int timer)
rc = pthread_cond_timedwait(&eventThreadJobCond, &eventThreadMutex,
&waitTime);
if (rc != 0) {
-virtTestResult(name, 1, "Timed out waiting for pipe event\n");
+testEventReport(name, 1, "Timed out waiting for pipe event\n");
return EXIT_FAILURE;
}
if (verifyFired(name, handle, timer) != EXIT_SUCCESS)
return EXIT_FAILURE;
-virtTestResult(name, 0, NULL);
+testEventReport(name, 0, NULL);
return EXIT_SUCCESS;
}
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] tests: sheepdog: Drop use of virtTestResult
Instead use the same pattern that most other test files use.
---
tests/storagebackendsheepdogtest.c | 63 --
1 file changed, 47 insertions(+), 16 deletions(-)
diff --git a/tests/storagebackendsheepdogtest.c
b/tests/storagebackendsheepdogtest.c
index 7744119..2b0f4db 100644
--- a/tests/storagebackendsheepdogtest.c
+++ b/tests/storagebackendsheepdogtest.c
@@ -44,15 +44,28 @@ typedef struct {
uint64_t expected_allocation;
} collie_test;
+struct testNodeInfoParserData {
+collie_test data;
+const char *poolxml;
+};
+
+struct testVDIListParserData {
+collie_test data;
+const char *poolxml;
+const char *volxml;
+};
+
static int
-test_node_info_parser(collie_test test, char *poolxml)
+test_node_info_parser(const void *opaque)
{
+const struct testNodeInfoParserData *data = opaque;
+collie_test test = data->data;
int ret = -1;
char *output = NULL;
virStoragePoolDefPtr pool = NULL;
-if (!(pool = virStoragePoolDefParseFile(poolxml)))
+if (!(pool = virStoragePoolDefParseFile(data->poolxml)))
goto cleanup;
if (VIR_STRDUP(output, test.output) < 0)
@@ -78,17 +91,19 @@ test_node_info_parser(collie_test test, char *poolxml)
}
static int
-test_vdi_list_parser(collie_test test, char *poolxml, char *volxml)
+test_vdi_list_parser(const void *opaque)
{
+const struct testVDIListParserData *data = opaque;
+collie_test test = data->data;
int ret = -1;
char *output = NULL;
virStoragePoolDefPtr pool = NULL;
virStorageVolDefPtr vol = NULL;
-if (!(pool = virStoragePoolDefParseFile(poolxml)))
+if (!(pool = virStoragePoolDefParseFile(data->poolxml)))
goto cleanup;
-if (!(vol = virStorageVolDefParseFile(pool, volxml, 0)))
+if (!(vol = virStorageVolDefParseFile(pool, data->volxml, 0)))
goto cleanup;
if (VIR_STRDUP(output, test.output) < 0)
@@ -118,7 +133,7 @@ test_vdi_list_parser(collie_test test, char *poolxml, char
*volxml)
static int
mymain(void)
{
-int ret = -1;
+int ret = 0;
char *poolxml = NULL;
char *volxml = NULL;
@@ -170,26 +185,42 @@ mymain(void)
abs_srcdir) < 0)
goto cleanup;
+#define DO_TEST_NODE(collie)\
+do {\
+struct testNodeInfoParserData data = { \
+.data = collie, \
+.poolxml = poolxml, \
+}; \
+if (virtTestRun("node_info_parser", test_node_info_parser, \
+&data) < 0) \
+ret = -1; \
+} while (0)
+
while (test->output != NULL) {
-ret = test_node_info_parser(*test, poolxml);
-virtTestResult("node_info_parser", ret, NULL);
+DO_TEST_NODE(*test);
++test;
-if (ret < 0)
-return EXIT_FAILURE;
}
+
+#define DO_TEST_VDI(collie) \
+do {\
+struct testVDIListParserData data = { \
+.data = collie, \
+.poolxml = poolxml, \
+.volxml = volxml, \
+}; \
+if (virtTestRun("vdi_list_parser", test_vdi_list_parser,\
+&data) < 0) \
+ret = -1; \
+} while (0)
+
test = vdi_list_tests;
while (test->output != NULL) {
-ret = test_vdi_list_parser(*test, poolxml, volxml);
-virtTestResult("vdi_list_parser", ret, NULL);
+DO_TEST_VDI(*test);
++test;
-if (ret < 0)
-return EXIT_FAILURE;
}
-ret = 0;
-
cleanup:
VIR_FREE(poolxml);
VIR_FREE(volxml);
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] testutils: Drop virtTestResult
virtTestResult is suboptimal for a few reasons:
- It poorly duplicates virtTestRun pass/fail reporting logic
- It doesn't have virtTestRun's alloc testing support
- It only reports the test name _after_ the test has run.
- It doesn't follow the standard virtTestRun pattern that most other
tests use.
There's no users left, so drop it. If any other async tests like eventtest
spring up that don't cleanly fit the virtTestRun pattern, I suggest they
just open code the support for it around virtTestRun
---
tests/testutils.c | 38 --
tests/testutils.h | 2 --
2 files changed, 40 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c
index bd4ff73..857e819 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -119,44 +119,6 @@ static void virTestAllocHook(int nalloc ATTRIBUTE_UNUSED,
}
#endif
-void virtTestResult(const char *name, int ret, const char *msg, ...)
-{
-va_list vargs;
-va_start(vargs, msg);
-
-if (testCounter == 0 && !virTestGetVerbose())
-fprintf(stderr, " ");
-
-testCounter++;
-if (virTestGetVerbose()) {
-fprintf(stderr, "%3zu) %-60s ", testCounter, name);
-if (ret == 0) {
-fprintf(stderr, "OK\n");
-} else {
-fprintf(stderr, "FAILED\n");
-if (msg) {
-char *str;
-if (virVasprintfQuiet(&str, msg, vargs) == 0) {
-fprintf(stderr, "%s", str);
-VIR_FREE(str);
-}
-}
-}
-} else {
-if (testCounter != 1 &&
-!((testCounter-1) % 40)) {
-fprintf(stderr, " %-3zu\n", (testCounter-1));
-fprintf(stderr, " ");
-}
-if (ret == 0)
-fprintf(stderr, ".");
-else
-fprintf(stderr, "!");
-}
-
-va_end(vargs);
-}
-
#ifdef TEST_OOM_TRACE
static void
virTestShowTrace(void)
diff --git a/tests/testutils.h b/tests/testutils.h
index f34a393..ccf1d29 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -48,8 +48,6 @@ extern char *progname;
bool virtTestOOMActive(void);
-void virtTestResult(const char *name, int ret, const char *msg, ...)
-ATTRIBUTE_FMT_PRINTF(3,4);
int virtTestRun(const char *title,
int (*body)(const void *data),
const void *data);
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] tests: Drop virtTestResult
virtTestResult poorly duplicates logic from virtTestRun. We could fix it, but there's really only one legitimate user in eventtest.c, so drop the improper users, and opencode the legit usage. More justification in the last patch Thanks, Cole Cole Robinson (3): tests: sheepdog: Drop use of virtTestResult tests: eventtest: Open code virtTestResult testutils: Drop virtTestResult tests/eventtest.c | 57 -- tests/storagebackendsheepdogtest.c | 63 -- tests/testutils.c | 38 --- tests/testutils.h | 2 -- 4 files changed, 94 insertions(+), 66 deletions(-) -- 2.5.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] testutils: Add coloring to verbose PASS/FAILED output
Helps to visually track down test failures if debugging the test suite.
The colors match what 'make check' does for pass/fail/skip
---
tests/testutils.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c
index 89026c6..bd4ff73 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -91,6 +91,11 @@ bool virtTestOOMActive(void)
return testOOMActive;
}
+static int virtTestUseTerminalColors(void)
+{
+return isatty(STDIN_FILENO);
+}
+
static unsigned int
virTestGetFlag(const char *name)
{
@@ -217,11 +222,20 @@ virtTestRun(const char *title,
if (virTestGetVerbose()) {
if (ret == 0)
-fprintf(stderr, "OK\n");
+if (virtTestUseTerminalColors())
+fprintf(stderr, "\e[32mOK\e[0m\n"); /* green */
+else
+fprintf(stderr, "OK\n");
else if (ret == EXIT_AM_SKIP)
-fprintf(stderr, "SKIP\n");
+if (virtTestUseTerminalColors())
+fprintf(stderr, "\e[34m\e[1mSKIP\e[0m\n"); /* bold blue */
+else
+fprintf(stderr, "SKIP\n");
else
-fprintf(stderr, "FAILED\n");
+if (virtTestUseTerminalColors())
+fprintf(stderr, "\e[31m\e[1mFAILED\e[0m\n"); /* bold red */
+else
+fprintf(stderr, "FAILED\n");
} else {
if (testCounter != 1 &&
!((testCounter-1) % 40)) {
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-python PATCH 03/23] remove useless check for NULL before Py_XDECREF
On 09/29/2015 10:29 AM, Pavel Hrdina wrote:
> On Sat, Sep 26, 2015 at 09:08:35AM -0400, John Ferlan wrote:
>>
>>
>> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
>>> Signed-off-by: Pavel Hrdina
>>> ---
>>> libvirt-override.c | 9 +++--
>>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libvirt-override.c b/libvirt-override.c
>>> index 14aa0e9..114104b 100644
>>> --- a/libvirt-override.c
>>> +++ b/libvirt-override.c
>>> @@ -2303,12 +2303,9 @@ libvirt_virRegisterErrorHandler(ATTRIBUTE_UNUSED
>>> PyObject * self,
>>>pyobj_f);
>>>
>>> virSetErrorFunc(NULL, libvirt_virErrorFuncHandler);
>>> -if (libvirt_virPythonErrorFuncHandler != NULL) {
>>> -Py_XDECREF(libvirt_virPythonErrorFuncHandler);
>>> -}
>>> -if (libvirt_virPythonErrorFuncCtxt != NULL) {
>>> -Py_XDECREF(libvirt_virPythonErrorFuncCtxt);
>>> -}
>>> +
>>> +Py_XDECREF(libvirt_virPythonErrorFuncHandler);
>>> +Py_XDECREF(libvirt_virPythonErrorFuncCtxt);
>>
>> I keep looking at this and thinking why? Why was it added and what is
>> it protecting. Looking at libvirt_virErrorFuncHandler it seems only
>> libvirt_virPythonErrorFuncCtxt gets the Py_XINCREF and that's based on
>> whether libvirt_virPythonErrorFuncHandler is NULL or not.
>
> I don't understand what do you mean by this??
>
I guess the whole Py_XDECREF usage is still a bit of a mystery. I
believe I was trying to consider why someone would have checked for NULL
previously. I know from checking it was written that way. But I assume
now that if on input it was NULL then nothing happens.
No issues otherwise -
John
>>
>> Just feels like something subtle is going on. Also it's a register
>> handler and we're decrementing something that never got incremented.
>
> Check the whole function. It will decrement the old handlers, check for
> existence of new handlers and clean/set new handlers. And before setting the
> new handlers, we need to increment them, otherwise they will be cleared by GC.
>
> Everything that is parsed by 'PyArg_ParseTuple' has to be incremented, if you
> need to use those object outside of the function scope, where you've called
> that
> parser.
>
> Pavel
>
>>
>> John
>>>
>>> if ((pyobj_f == Py_None) && (pyobj_ctx == Py_None)) {
>>> libvirt_virPythonErrorFuncHandler = NULL;
>>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-php] free xml resources in get_string_from_xpath
free as much as possible on return from get_string_from_xpath
Signed-off-by: Vasiliy Tolstov
---
src/libvirt-php.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/libvirt-php.c b/src/libvirt-php.c
index 8588128..18499a6 100644
--- a/src/libvirt-php.c
+++ b/src/libvirt-php.c
@@ -2597,6 +2597,7 @@ char *get_string_from_xpath(char *xml, char *xpath, zval
**val, int *retVal)
if (!doc) {
if (retVal)
*retVal = -2;
+xmlFreeParserCtxt(xp);
xmlCleanupParser();
return NULL;
}
@@ -2605,6 +2606,8 @@ char *get_string_from_xpath(char *xml, char *xpath, zval
**val, int *retVal)
if (!context) {
if (retVal)
*retVal = -3;
+xmlFreeDoc(doc);
+xmlFreeParserCtxt(xp);
xmlCleanupParser();
return NULL;
}
@@ -2614,6 +2617,8 @@ char *get_string_from_xpath(char *xml, char *xpath, zval
**val, int *retVal)
if (retVal)
*retVal = -4;
xmlXPathFreeContext(context);
+xmlFreeParserCtxt(xp);
+xmlFreeDoc(doc);
xmlCleanupParser();
return NULL;
}
@@ -2621,6 +2626,8 @@ char *get_string_from_xpath(char *xml, char *xpath, zval
**val, int *retVal)
if(xmlXPathNodeSetIsEmpty(result->nodesetval)){
xmlXPathFreeObject(result);
xmlXPathFreeContext(context);
+xmlFreeParserCtxt(xp);
+xmlFreeDoc(doc);
xmlCleanupParser();
if (retVal)
*retVal = 0;
@@ -2632,8 +2639,9 @@ char *get_string_from_xpath(char *xml, char *xpath, zval
**val, int *retVal)
if (ret == 0) {
xmlXPathFreeObject(result);
-xmlFreeDoc(doc);
xmlXPathFreeContext(context);
+xmlFreeParserCtxt(xp);
+xmlFreeDoc(doc);
xmlCleanupParser();
if (retVal)
*retVal = 0;
@@ -2658,8 +2666,9 @@ char *get_string_from_xpath(char *xml, char *xpath, zval
**val, int *retVal)
value = (char *)xmlNodeListGetString(doc,
nodeset->nodeTab[0]->xmlChildrenNode, 1);
}
-xmlXPathFreeContext(context);
xmlXPathFreeObject(result);
+xmlXPathFreeContext(context);
+xmlFreeParserCtxt(xp);
xmlFreeDoc(doc);
xmlCleanupParser();
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainAttachDeviceLive: Check provided disk address
On 09/29/2015 11:03 AM, Michal Privoznik wrote:
> On 25.09.2015 14:45, Martin Kletzander wrote:
>> On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote:
>>>
>>>
>>> On 09/25/2015 05:38 AM, Michal Privoznik wrote:
On 25.09.2015 11:36, Martin Kletzander wrote:
> On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1257844
>>
>> Imagine an user who is trying to attach a disk to a domain with
>> the following XML:
>>
>>
>>
>>
>>
>>
>>
>>
>> The XML is obviously wrong. It's trying to attach a virtio disk
>> onto non-PCI bus. We should forbid that.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_hotplug.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>
> How come this is not handled in qemuDomainAssignAddresses(), it
> doesn't get called? There's a check for exactly that in
> qemuAssignDevicePCISlots().
Exactly! qemuAssignDevicePCISlots() is called only in case of --config.
>>>
>>> Seems to me this may be more of a generic problem - a user providing the
>>> wrong address type for the type of device. I have a recollection of
>>
>> Yes, and since we have checks for those, it's confusing to me why
>> would qemuAssignDevicePCISlots() be called only with --config. Can we
>> use that code which checks for more things already? For example, the
>> here-missing virtio-mmio.
>
> Yes and no. qemuDomainAssignAddresses() expects to see vm->def which
> already has the device attached. Moreover, it not only works over all
> devices in the domain (filling in all the missing addresses), but
> possibly creating new controllers too (e.g. plugging new pci bridges if
> running out of addresses on a bus).
>
> Does anybody have a bright idea how this can be fixed apart from obvious
> one - breaking whole address assignment code into parts and reassembling
> it back together again?
>
>From the v4 from Ruifeng Beng:
http://www.redhat.com/archives/libvir-list/2015-September/msg00524.html
which has some validity w/r/t using qemuCheckDiskConfig; however, I'm
wondering now if that's far too late in processing. Is it agreeable that
the checks in virDomainDeviceAddressTypeIsValid will cover the cases
when the incoming device has an address in the XML?
Why not use the power of virDomainDeviceDefPostParseInternal? There's a
check in that code:
if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0)
return -1;
why not make an "else" which says, let's be sure the disk->info.type
provided is valid?
I've attached a patch which works for the cases shown in the bz (both no
params and --config).
This would solve attach, update, and detach since each would call the
virDomainDeviceDefParse which calls the PostParse code.
John
NOTE: The change to the test is because the failure now occurs during
parse rather than at run (e.g. earlier, where I think it should).
>From a03a8aa073eb410d6b713e6f77572edcf0631263 Mon Sep 17 00:00:00 2001
From: John Ferlan
Date: Tue, 29 Sep 2015 17:04:11 -0400
Subject: [PATCH] bug 1257844
Signed-off-by: John Ferlan
---
src/conf/domain_conf.c | 43 +++
tests/qemuxml2argvtest.c | 2 +-
2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 393ece7..21439c7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3962,6 +3962,41 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
}
+/* Check if a provided address is valid */
+static bool
+virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
+{
+if (disk->info.type) {
+switch (disk->bus) {
+case VIR_DOMAIN_DISK_BUS_IDE:
+case VIR_DOMAIN_DISK_BUS_SCSI:
+case VIR_DOMAIN_DISK_BUS_SATA:
+case VIR_DOMAIN_DISK_BUS_FDC:
+if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
+return false;
+break;
+case VIR_DOMAIN_DISK_BUS_USB:
+if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
+return false;
+break;
+case VIR_DOMAIN_DISK_BUS_VIRTIO:
+if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
+disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 &&
+disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
+disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+return false;
+break;
+case VIR_DOMAIN_DISK_BUS_XEN:
+case VIR_DOMAIN_DISK_BUS_UML:
+case VIR_DOMAIN_DISK_BUS_SD:
+/* no address type currently, return false if address provided */
+return false;
+}
+}
+return true;
+}
+
+
/* Check if a drive type
Re: [libvirt] [PATCH v4] qemu: Validate address type when attaching a disk device.
On 09/17/2015 03:35 AM, Ruifeng Bian wrote:
> Bug fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1257844
>
> Attach-device can hotplug a virtio disk device with any address type now,
> it need to validate the address type before the attachment.
>
> Attaching a disk device with --config option need to check address type.
> Otherwise, the domain cloudn't be started normally. This patch add a basic
> check for address type.
>
> Detaching a disk device with --config also need to check disk options,
> add qemuCheckDiskConfig() method in qemuDomainDetachDeviceConfig().
>
> Add virDomainDeviceAddressTypeIsValid method in domain_conf to check disk
> address type. Address type should match with disk driver, report error
> messages if any mismatch.
>
> Signed-off-by: Ruifeng Bian
> ---
> src/conf/domain_conf.c | 32
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_command.c | 7 +++
> src/qemu/qemu_driver.c | 2 ++
> src/qemu/qemu_hotplug.c | 22 ++
> 6 files changed, 65 insertions(+)
>
Just so this isn't left "hanging"... Michal has generated an alternate
method to address the issue, see:
http://www.redhat.com/archives/libvir-list/2015-September/msg00909.html
I think there could be "synergies" between the two and left some
thoughts in the other patch which use some of this code.
Some other comments below since they may be useful...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6df1618..f86760b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3200,6 +3200,38 @@ int
> virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
> return 0;
> }
>
> +int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
should be:
bool
virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
> +{
> +if (disk->info.type) {
> +switch (disk->bus) {
> +case VIR_DOMAIN_DISK_BUS_IDE:
> +case VIR_DOMAIN_DISK_BUS_SCSI:
> +case VIR_DOMAIN_DISK_BUS_SATA:
> +case VIR_DOMAIN_DISK_BUS_FDC:
> +if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
> +return 1;
return false;
> +break;
> +case VIR_DOMAIN_DISK_BUS_USB:
> +if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> +return 1;
return false;
> +break;
> +case VIR_DOMAIN_DISK_BUS_VIRTIO:
> +if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> +disk->info.type !=
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 &&
> +disk->info.type !=
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
> +disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
> +return 1;
return false;
> +break;
> +case VIR_DOMAIN_DISK_BUS_XEN:
> +case VIR_DOMAIN_DISK_BUS_UML:
> +case VIR_DOMAIN_DISK_BUS_SD:
> +/* no address type currently, return false if address provided */
> +return 1;
return false;
> +}
> +}
> +return 0;
return true;
> +}
> +
> virDomainDeviceInfoPtr
> virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
> {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index f043554..337fe51 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2543,6 +2543,7 @@ virDomainDeviceDefPtr
> virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
> virDomainXMLOptionPtr xmlopt);
> int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
>int type);
> +int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk);
s/int/bool, probably unnecessary though
> virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device);
> int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
> virDomainDeviceInfoPtr src);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5b3da83..6cd5b9e 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -229,6 +229,7 @@ virDomainDefPostParse;
> virDomainDefSetMemoryInitial;
> virDomainDeleteConfig;
> virDomainDeviceAddressIsValid;
> +virDomainDeviceAddressTypeIsValid;
probably unnecessary
> virDomainDeviceAddressTypeToString;
> virDomainDeviceDefCheckUnsupportedMemoryDevice;
> virDomainDeviceDefCopy;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 25f57f2..56ba08d 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3606,6 +3606,13 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
> goto error;
> }
> }
> +
> +if (virDomainDeviceAddressTypeIsValid(disk)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk cannot have an address of type '%s'"),
> +
Re: [libvirt] [PATCH 1/2] qemu: Perform the disk WWN check only on fresh starts
On Tue, Sep 29, 2015 at 18:38:04 +0200, Peter Krempa wrote: > Since we'd disallow migration of a guest that would have possibly > invalid config but still be able to work, relax the WWN check to be > performed only on new starts of the VM. > --- > src/qemu/qemu_process.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index f2586a1..9c107bd 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -4628,7 +4628,8 @@ int qemuProcessStart(virConnectPtr conn, > goto cleanup; > } > > -if (virDomainDefCheckDuplicateDiskWWN(vm->def) < 0) > +if (!migrateFrom && !snapshot && > +virDomainDefCheckDuplicateDiskWWN(vm->def) < 0) > goto cleanup; > > /* "volume" type disk's source must be translated before In other words, this will allow domains started on an older libvirt (which didn't check for duplicate disk info) to be migrated to or restored on a new libvirtd, right? ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainAttachDeviceLive: Check provided disk address
On Tue, Sep 29, 2015 at 05:03:07PM +0200, Michal Privoznik wrote: On 25.09.2015 14:45, Martin Kletzander wrote: On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote: On 09/25/2015 05:38 AM, Michal Privoznik wrote: On 25.09.2015 11:36, Martin Kletzander wrote: On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote: https://bugzilla.redhat.com/show_bug.cgi?id=1257844 Imagine an user who is trying to attach a disk to a domain with the following XML: The XML is obviously wrong. It's trying to attach a virtio disk onto non-PCI bus. We should forbid that. Signed-off-by: Michal Privoznik --- src/qemu/qemu_hotplug.c | 7 +++ 1 file changed, 7 insertions(+) How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots(). Exactly! qemuAssignDevicePCISlots() is called only in case of --config. Seems to me this may be more of a generic problem - a user providing the wrong address type for the type of device. I have a recollection of Yes, and since we have checks for those, it's confusing to me why would qemuAssignDevicePCISlots() be called only with --config. Can we use that code which checks for more things already? For example, the here-missing virtio-mmio. Yes and no. qemuDomainAssignAddresses() expects to see vm->def which already has the device attached. Moreover, it not only works over all devices in the domain (filling in all the missing addresses), but possibly creating new controllers too (e.g. plugging new pci bridges if running out of addresses on a bus). Does anybody have a bright idea how this can be fixed apart from obvious one - breaking whole address assignment code into parts and reassembling it back together again? Unfortunately, not. Well, maybe the address allocation I was trying to push as GSoC and/or Outreachy project could've fixed that, but nobody was able to finish that, so we don't know =) Michal signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] kvm-all: notice KVM of vcpu's TSC rate after migration
On Tue, Sep 29, 2015 at 11:43:34AM +0800, Haozhong Zhang wrote:
> On Mon, Sep 28, 2015 at 01:37:34PM -0300, Eduardo Habkost wrote:
> > On Mon, Sep 28, 2015 at 01:38:31PM +0800, Haozhong Zhang wrote:
[...]
> > > static void do_kvm_cpu_synchronize_post_init(void *arg)
> > > {
> > > CPUState *cpu = arg;
> > > +CPUX86State *env = &X86_CPU(cpu)->env;
> > > +int r;
> > > +
> > > +/*
> > > + * XXX: KVM_SET_TSC_KHZ must be done before kvm_arch_put_registers().
> >
> > Could you explain where this requirement comes from?
> >
>
> kvm_arch_put_registers() below will setup vcpu's MSR_IA32_TSC through
> KVM ioctl KVM_SET_MSRS. KVM needs to know vcpu's TSC rate so as to
> correctly scale the TSC value given by QEMU, especially when vcpu's
> TSC rate is different than the host TSC rate (e.g. it's migrated from
> another machine w/ different host TSC rate than the current one).
Thanks. The comment above could contain a short version of this
explanation, e.g.: "KVM needs KVM_SET_TSC_KHZ to be done before
KVM_SET_MSRS sets MSR_IA32_TSC (done by kvm_arch_put_registers())".
>
[...]
> > Or maybe we shouldn't treat this as VM state, but as configuration, and
> > let management configure the TSC frequency explicitly if the user really
> > needs it to stay the same during migration.
> >
> > (CCing libvir-list to see if they have feedback)
> >
>
> Thanks for CC. I'll consider to add a command line option to control
> the configuration of guest TSC fequency.
It already exists, -cpu has a "tsc-freq" option.
--
Eduardo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] network: avoid existing bridges during rpm install of default network
When we install the libvirt-daemon-config-network package from an rpm,
the specfile checks that the subnet used for the default network isn't
already used, but blindly assumes that virbr0 is available. This is
almost always the case, but there could be a situation where someone
was already using virbr0 for their own network, then decided to
install libvirt-daemon-config-network, leading to a failure when they
tried to start the default network.
This patch adds a bit to the %post script for the
daemon-network-config package (in the specfile, used only for .rpm
packages) that checks "ip link show" and the existing libvirt network
xml files in a loop to find the lowest numbered virbrN that is
currently unused by either.
(note that we already check for in-use bridge devices when defining a
network in libvirt's network driver, but the rpm install bypasses
libvirt (which may not yet be fully functional) and creates the xml
file itself).
---
I found this sitting in a source tree, written a few months ago and
forgotten. It is a followup to commit 37b8bc6f, which added a similar
check in the network driver during virNetworkDefineXML(). I vaguely
recall someone reporting this problem on IRC or maybe on one of the
mailing lists (they had created a bridge manually using the name
virbr0, then when they installed libvirt, it happily made a network
definition using virbr0). Yes, not very common, but possible.
libvirt.spec.in | 16
1 file changed, 16 insertions(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 78a4cc3..931c5b9 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1793,8 +1793,24 @@ if test $1 -eq 1 && test ! -f
%{_sysconfdir}/libvirt/qemu/networks/default.xml ;
;;
esac
+# find an unused bridge device named virbrN (this assumes that the
+# original name in default.xml is exactly "virbr0", so do not
+# change it there without also changing orig_br).
+orig_br=0
+br=${orig_br}
+while ip link show virbr${br} >/dev/null 2>&1 ||\
+ (grep '/dev/null 2>&1); do
+ br=$(expr ${br} + 1)
+ # safety to prevent an endless loop
+ if test ${br} -gt 256; then
+ break;
+ fi
+done
+
UUID=`/usr/bin/uuidgen`
sed -e "s/${orig_sub}/${sub}/g" \
+-e "s/virbr${orig_br}/virbr${br}/" \
-e "s,,\n $UUID," \
< %{_datadir}/libvirt/networks/default.xml \
> %{_sysconfdir}/libvirt/qemu/networks/default.xml
--
2.4.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] conf: Reuse virDomainDefCheckDuplicateDiskWWN to check disk serial too
On Tue, Sep 29, 2015 at 18:50:48 +0200, Jiri Denemark wrote:
> On Tue, Sep 29, 2015 at 18:38:05 +0200, Peter Krempa wrote:
> > Rename the function to virDomainDefCheckDuplicateDiskInfo and make it
> > check disk serials too.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1245013
> > ---
> > src/conf/domain_conf.c | 15 ++-
> > src/conf/domain_conf.h | 2 +-
> > src/libvirt_private.syms | 2 +-
> > src/qemu/qemu_process.c | 2 +-
> > 4 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 393ece7..e9d61db 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -24451,7 +24451,7 @@ virDomainDefNeedsPlacementAdvice(virDomainDefPtr
> > def)
> >
> >
> > int
> > -virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def)
> > +virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def)
> > {
> > size_t i;
> > size_t j;
> > @@ -24469,6 +24469,19 @@ virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr
> > def)
> > }
> > }
> > }
> > +
> > +if (def->disks[i]->serial) {
> > +for (j = i + 1; j < def->ndisks; j++) {
> > +if (STREQ_NULLABLE(def->disks[i]->serial,
> > + def->disks[j]->serial)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("Disks '%s' and '%s' have identical
> > serial"),
> > + def->disks[i]->dst,
> > + def->disks[j]->dst);
> > +return -1;
> > +}
> > +}
> > +}
>
> Any reason for going through all disks twice? Wouldn't it be better to
> go through them once and check for both serial and wwn within a single
> loop? :-)
Erm. I'm going to blame me writing suboptimal (read: crappy) code on
hunger.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] conf: Reuse virDomainDefCheckDuplicateDiskWWN to check disk serial too
On Tue, Sep 29, 2015 at 18:38:05 +0200, Peter Krempa wrote:
> Rename the function to virDomainDefCheckDuplicateDiskInfo and make it
> check disk serials too.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1245013
> ---
> src/conf/domain_conf.c | 15 ++-
> src/conf/domain_conf.h | 2 +-
> src/libvirt_private.syms | 2 +-
> src/qemu/qemu_process.c | 2 +-
> 4 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 393ece7..e9d61db 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -24451,7 +24451,7 @@ virDomainDefNeedsPlacementAdvice(virDomainDefPtr def)
>
>
> int
> -virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def)
> +virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def)
> {
> size_t i;
> size_t j;
> @@ -24469,6 +24469,19 @@ virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr
> def)
> }
> }
> }
> +
> +if (def->disks[i]->serial) {
> +for (j = i + 1; j < def->ndisks; j++) {
> +if (STREQ_NULLABLE(def->disks[i]->serial,
> + def->disks[j]->serial)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Disks '%s' and '%s' have identical
> serial"),
> + def->disks[i]->dst,
> + def->disks[j]->dst);
> +return -1;
> +}
> +}
> +}
Any reason for going through all disks twice? Wouldn't it be better to
go through them once and check for both serial and wwn within a single
loop? :-)
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-python PATCH 18/23] improve usage of cleanup paths
On Sat, Sep 26, 2015 at 10:14:37AM -0400, John Ferlan wrote:
>
>
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > This removes several code duplicates and also some unusual code structures.
> >
> > Signed-off-by: Pavel Hrdina
> > ---
> > libvirt-override.c | 159
> > +++--
> > 1 file changed, 68 insertions(+), 91 deletions(-)
> >
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 8afe7d7..205f0dd 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -439,13 +439,13 @@ libvirt_virDomainGetSchedulerType(PyObject *self
> > ATTRIBUTE_UNUSED,
> > return VIR_PY_NONE;
> >
> > /* convert to a Python tuple of long objects */
> > -if ((info = PyTuple_New(2)) == NULL) {
> > -VIR_FREE(c_retval);
> > -return NULL;
> > -}
> > +if ((info = PyTuple_New(2)) == NULL)
> > +goto cleanup;
> >
> > PyTuple_SetItem(info, 0, libvirt_constcharPtrWrap(c_retval));
> > PyTuple_SetItem(info, 1, libvirt_intWrap((long)nparams));
> > +
> > + cleanup:
> > VIR_FREE(c_retval);
> > return info;
> > }
> > @@ -2401,7 +2401,6 @@ libvirt_virDomainSnapshotListNames(PyObject *self
> > ATTRIBUTE_UNUSED,
> > Py_CLEAR(py_retval);
> > goto cleanup;
> > }
> > -VIR_FREE(names[i]);
> > }
> >
> > cleanup:
> > @@ -3245,8 +3244,8 @@ libvirt_virNodeGetCellsFreeMemory(PyObject *self
> > ATTRIBUTE_UNUSED,
> > LIBVIRT_END_ALLOW_THREADS;
> >
> > if (c_retval < 0) {
> > -VIR_FREE(freeMems);
> > -return VIR_PY_NONE;
> > +py_retval = VIR_PY_NONE;
> > +goto cleanup;
> > }
> >
> > if ((py_retval = PyList_New(c_retval)) == NULL)
> > @@ -3425,24 +3424,19 @@ libvirt_virConnectListStoragePools(PyObject *self
> > ATTRIBUTE_UNUSED,
> > return VIR_PY_NONE;
> > }
> > }
> > -py_retval = PyList_New(c_retval);
> > -if (py_retval == NULL) {
> > -if (names) {
> > -for (i = 0; i < c_retval; i++)
> > -VIR_FREE(names[i]);
> > -VIR_FREE(names);
> > -}
> > -return NULL;
> > -}
> > +
> > +if ((py_retval = PyList_New(c_retval)) == NULL)
> > +goto cleanup;
> >
> > if (names) {
> > -for (i = 0; i < c_retval; i++) {
> > +for (i = 0; i < c_retval; i++)
> > PyList_SetItem(py_retval, i,
> > libvirt_constcharPtrWrap(names[i]));
> > -VIR_FREE(names[i]);
> > -}
> > -VIR_FREE(names);
> > }
> >
> > + cleanup:
> > +for (i = 0; i < c_retval; i++)
> > +VIR_FREE(names[i]);
> > +VIR_FREE(names);
> > return py_retval;
> > }
> >
> > @@ -3480,24 +3474,20 @@ libvirt_virConnectListDefinedStoragePools(PyObject
> > *self ATTRIBUTE_UNUSED,
> > return VIR_PY_NONE;
> > }
> > }
> > -py_retval = PyList_New(c_retval);
> > -if (py_retval == NULL) {
> > -if (names) {
> > -for (i = 0; i < c_retval; i++)
> > -VIR_FREE(names[i]);
> > -VIR_FREE(names);
> > -}
> > -return NULL;
> > -}
> > +
> > +if ((py_retval = PyList_New(c_retval)) == NULL)
> > +goto cleanup;
> >
> > if (names) {
> > -for (i = 0; i < c_retval; i++) {
> > +for (i = 0; i < c_retval; i++)
> > PyList_SetItem(py_retval, i,
> > libvirt_constcharPtrWrap(names[i]));
> > -VIR_FREE(names[i]);
> > -}
> > -VIR_FREE(names);
> > }
> >
> > + cleanup:
> > +for (i = 0; i < c_retval; i++)
> > +VIR_FREE(names[i]);
> > +VIR_FREE(names);
> > +
> > return py_retval;
> > }
> >
> > @@ -3579,28 +3569,24 @@ libvirt_virStoragePoolListVolumes(PyObject *self
> > ATTRIBUTE_UNUSED,
> > c_retval = virStoragePoolListVolumes(pool, names, c_retval);
> > LIBVIRT_END_ALLOW_THREADS;
> > if (c_retval < 0) {
> > -VIR_FREE(names);
> > -return VIR_PY_NONE;
> > +py_retval = VIR_PY_NONE;
> > +goto cleanup;
>
> We're going to cleanup with c_retval == -1, right... and then doing the
> for loop to free names - could be a long time to complete.
>
> I do note that this differs from other prior uses in this patch - that
> is other functions will keep the VIR_FREE(names); return VIR_PY_NONE; if
> c_retval < 0
>
> At this point it may be worthwhile for you to check *all* such goto's
> for c_retval && loops to free memory...
Oh, I didn't realize this, because some functions have 'int i;' and others have
'size_t i'. I've verified the 'i' declaration only in function with int and
conclude it's ok to use it this way. This needs to be fixed, I'll send a v2.
Pavel
>
> > }
> > }
> > -py_retval = PyList_New(c_retval);
> > -if (py_retval == NULL) {
> > -if (names) {
> > -for (i = 0; i < c_retval; i++)
>
Re: [libvirt] [libvirt-python PATCH 22/23] use VIR_PY_DICT_SET_GOTO
On Sat, Sep 26, 2015 at 10:46:13AM -0400, John Ferlan wrote:
>
>
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina
> > ---
> > libvirt-override.c | 275
> > +++--
> > libvirt-utils.c| 13 +--
> > 2 files changed, 99 insertions(+), 189 deletions(-)
> >
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 1f795d9..3192e74 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -403,18 +403,14 @@ libvirt_virDomainMemoryStats(PyObject *self
> > ATTRIBUTE_UNUSED,
> > }
> > val = libvirt_ulonglongWrap(stats[i].val);
> >
> > -if (!key || !val || PyDict_SetItem(info, key, val) < 0) {
> > -Py_CLEAR(info);
> > -goto cleanup;
> > -}
> > -Py_CLEAR(key);
> > -Py_CLEAR(val);
> > +VIR_PY_DICT_SET_GOTO(info, key, val, error);
> > }
> >
> > - cleanup:
> > -Py_XDECREF(key);
> > -Py_XDECREF(val);
> > return info;
> > +
> > + error:
> > +Py_DECREF(info);
> > +return NULL;
> > }
> >
> > static PyObject *
> > @@ -3358,23 +3354,16 @@ libvirt_virNodeGetCPUStats(PyObject *self
> > ATTRIBUTE_UNUSED,
> > key = libvirt_constcharPtrWrap(stats[i].field);
> > val = libvirt_ulonglongWrap(stats[i].value);
> >
> > -if (!key || !val || PyDict_SetItem(ret, key, val) < 0) {
> > -Py_CLEAR(ret);
> > -goto error;
> > -}
> > -
> > -Py_DECREF(key);
> > -Py_DECREF(val);
> > +VIR_PY_DICT_SET_GOTO(ret, key, val, error);
> > }
> >
> > + cleanup:
> > VIR_FREE(stats);
> > return ret;
> >
> > error:
> > -VIR_FREE(stats);
> > -Py_XDECREF(key);
> > -Py_XDECREF(val);
> > -return ret;
> > +Py_CLEAR(ret);
> > +goto cleanup;
> > }
> >
> > static PyObject *
> > @@ -3423,23 +3412,16 @@ libvirt_virNodeGetMemoryStats(PyObject *self
> > ATTRIBUTE_UNUSED,
> > key = libvirt_constcharPtrWrap(stats[i].field);
> > val = libvirt_ulonglongWrap(stats[i].value);
> >
> > -if (!key || !val || PyDict_SetItem(ret, key, val) < 0) {
> > -Py_CLEAR(ret);
> > -goto error;
> > -}
> > -
> > -Py_DECREF(key);
> > -Py_DECREF(val);
> > +VIR_PY_DICT_SET_GOTO(ret, key, val, error);
> > }
> >
> > + cleanup:
> > VIR_FREE(stats);
> > return ret;
> >
> > error:
> > -VIR_FREE(stats);
> > -Py_XDECREF(key);
> > -Py_XDECREF(val);
> > -return ret;
> > +Py_CLEAR(ret);
> > +goto cleanup;
> > }
> >
> > static PyObject *
> > @@ -4747,11 +4729,8 @@ libvirt_virDomainGetJobStats(PyObject *self
> > ATTRIBUTE_UNUSED,
> > if (!(dict = getPyVirTypedParameter(params, nparams)))
> > goto cleanup;
> >
> > -if (PyDict_SetItem(dict, libvirt_constcharPtrWrap("type"),
> > - libvirt_intWrap(type)) < 0) {
> > -Py_CLEAR(dict);
> > -goto cleanup;
> > -}
> > +VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("type"),
> > + libvirt_intWrap(type), cleanup);
>
> it's not clear in this case on error that there's a "Py_CLEAR(dict);" -
> think you may need a goto error which has the Py_CLEAR(dict) and goto
> cleanup
Right, I'll fix that.
>
> >
> > cleanup:
> > virTypedParamsFree(params, nparams);
> > @@ -4790,34 +4769,19 @@ libvirt_virDomainGetBlockJobInfo(PyObject *self
> > ATTRIBUTE_UNUSED,
> > if (c_ret == 0)
> > return dict;
> >
> > -if ((type = libvirt_intWrap(info.type)) == NULL ||
> > -PyDict_SetItemString(dict, "type", type) < 0)
> > -goto error;
> > -Py_DECREF(type);
> > -
> > -if ((bandwidth = libvirt_ulongWrap(info.bandwidth)) == NULL ||
> > -PyDict_SetItemString(dict, "bandwidth", bandwidth) < 0)
> > -goto error;
> > -Py_DECREF(bandwidth);
> > -
> > -if ((cur = libvirt_ulonglongWrap(info.cur)) == NULL ||
> > -PyDict_SetItemString(dict, "cur", cur) < 0)
> > -goto error;
> > -Py_DECREF(cur);
> > -
> > -if ((end = libvirt_ulonglongWrap(info.end)) == NULL ||
> > -PyDict_SetItemString(dict, "end", end) < 0)
> > -goto error;
> > -Py_DECREF(end);
> > +VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("type"),
> > + libvirt_intWrap(info.type), error);
> > +VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("bandwidth"),
> > + libvirt_ulongWrap(info.bandwidth), error);
> > +VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("cur"),
> > + libvirt_ulonglongWrap(info.cur), error);
> > +VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("end"),
> > + libvirt_ulonglongWrap(info.end), error);
>
> build issue - 'end', 'cur', 'bandwidth', and 'type' no longer necessary
> >
> > return dict;
> >
> > e
[libvirt] [PATCH 2/2] conf: Reuse virDomainDefCheckDuplicateDiskWWN to check disk serial too
Rename the function to virDomainDefCheckDuplicateDiskInfo and make it
check disk serials too.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1245013
---
src/conf/domain_conf.c | 15 ++-
src/conf/domain_conf.h | 2 +-
src/libvirt_private.syms | 2 +-
src/qemu/qemu_process.c | 2 +-
4 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 393ece7..e9d61db 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24451,7 +24451,7 @@ virDomainDefNeedsPlacementAdvice(virDomainDefPtr def)
int
-virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def)
+virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def)
{
size_t i;
size_t j;
@@ -24469,6 +24469,19 @@ virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def)
}
}
}
+
+if (def->disks[i]->serial) {
+for (j = i + 1; j < def->ndisks; j++) {
+if (STREQ_NULLABLE(def->disks[i]->serial,
+ def->disks[j]->serial)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Disks '%s' and '%s' have identical
serial"),
+ def->disks[i]->dst,
+ def->disks[j]->dst);
+return -1;
+}
+}
+}
}
return 0;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a101e2a..fd4ef82 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3219,7 +3219,7 @@ virDomainParseMemory(const char *xpath,
bool virDomainDefNeedsPlacementAdvice(virDomainDefPtr def)
ATTRIBUTE_NONNULL(1);
-int virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def)
+int virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def)
ATTRIBUTE_NONNULL(1);
#endif /* __DOMAIN_CONF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c87efa1..c1b7b87 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -200,7 +200,7 @@ virDomainCpuPlacementModeTypeFromString;
virDomainCpuPlacementModeTypeToString;
virDomainDefAddImplicitControllers;
virDomainDefCheckABIStability;
-virDomainDefCheckDuplicateDiskWWN;
+virDomainDefCheckDuplicateDiskInfo;
virDomainDefCheckUnsupportedMemoryHotplug;
virDomainDefClearCCWAddresses;
virDomainDefClearDeviceAliases;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9c107bd..8cd713f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4629,7 +4629,7 @@ int qemuProcessStart(virConnectPtr conn,
}
if (!migrateFrom && !snapshot &&
-virDomainDefCheckDuplicateDiskWWN(vm->def) < 0)
+virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
goto cleanup;
/* "volume" type disk's source must be translated before
--
2.4.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Check disk serial strings for uniqueness
Peter Krempa (2): qemu: Perform the disk WWN check only on fresh starts conf: Reuse virDomainDefCheckDuplicateDiskWWN to check disk serial too src/conf/domain_conf.c | 15 ++- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/qemu/qemu_process.c | 3 ++- 4 files changed, 18 insertions(+), 4 deletions(-) -- 2.4.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] qemu: Perform the disk WWN check only on fresh starts
Since we'd disallow migration of a guest that would have possibly invalid config but still be able to work, relax the WWN check to be performed only on new starts of the VM. --- src/qemu/qemu_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f2586a1..9c107bd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4628,7 +4628,8 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; } -if (virDomainDefCheckDuplicateDiskWWN(vm->def) < 0) +if (!migrateFrom && !snapshot && +virDomainDefCheckDuplicateDiskWWN(vm->def) < 0) goto cleanup; /* "volume" type disk's source must be translated before -- 2.4.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH tck 0/5] Getting the TCK to run against opensuse images
On Tue, Sep 29, 2015 at 04:48:46PM +0200, Cédric Bosdonnat wrote: > Hi all, > > Here are a few patches to get the TCK run properly against opensuse image. > The patch adding the opensuse images to virt-builder is still pending, but > the main issues were: > > * Waiting for 30 seconds for the guest to be started is pretty fragile. > For the tests pinging the guest, wait for domifstats to report 10 sent > packets. > * The first boot of the opensuse image runs dracut and takes quite a long > time. We need to wait for the first boot to complete before shutting > down the guest to avoid inconsistent states. > * Yum isn't in all distros by default you know ;) > > Cédric Bosdonnat (5): > Ping test: use domifstats to wait for dhcp to be setup in guest > Create has_disk_image from code of get_virt_builder_disk > Add DomainBuilder->rminterface function. > Make sure the image has been started properly started the first time. > 240-no-arp-spoofing.t: get virt-builder install dsniff ACK to all patches Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-python PATCH 16/23] change the order of some statements
On Sat, Sep 26, 2015 at 09:56:03AM -0400, John Ferlan wrote:
>
>
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > This change makes it easier to free allocated object especially for
> > python objects. We can benefit from the fact, that if you call
> > Py_DECREF on eny python object it will also remove reference for all
>
> s/eny/any
>
> > assigned object to the root object. For example, calling Py_DECREF on
> > dict will also remove reference recursively on all elements in that
> > dictionary. Our job is then just call Py_DECREF on the root element and
> > don't care about anything else.
> >
>
> Something else that could go in HACKING - usage of Py_[X]DECREF...
That's true, I can probably updated it during second "phase" of improving
libvirt-python. I'm planning to improve the generator in order to automatically
generate more APIs.
>
> > Signed-off-by: Pavel Hrdina
> > ---
> > libvirt-override.c | 189
> > -
> > 1 file changed, 85 insertions(+), 104 deletions(-)
> >
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 92c31b8..63a469b 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -1238,9 +1238,16 @@ libvirt_virDomainGetVcpus(PyObject *self
> > ATTRIBUTE_UNUSED,
> > goto cleanup;
> > if ((pycpuinfo = PyList_New(dominfo.nrVirtCpu)) == NULL)
> > goto cleanup;
> > +
> > +if (PyTuple_SetItem(pyretval, 0, pycpuinfo) < 0)
> > +goto cleanup;
> > +
> > if ((pycpumap = PyList_New(dominfo.nrVirtCpu)) == NULL)
> > goto cleanup;
> >
> > +if (PyTuple_SetItem(pyretval, 1, pycpumap) < 0)
> > +goto cleanup;
> > +
> > for (i = 0; i < dominfo.nrVirtCpu; i++) {
> > PyObject *info = PyTuple_New(4);
> > PyObject *item = NULL;
> > @@ -1248,54 +1255,42 @@ libvirt_virDomainGetVcpus(PyObject *self
> > ATTRIBUTE_UNUSED,
> > if (info == NULL)
> > goto cleanup;
> >
> > +if (PyList_SetItem(pycpuinfo, i, info) < 0)
> > +goto cleanup;
> > +
> > if ((item = libvirt_intWrap((long)cpuinfo[i].number)) == NULL ||
> > PyTuple_SetItem(info, 0, item) < 0)
> > -goto itemError;
> > +goto cleanup;
> >
> > if ((item = libvirt_intWrap((long)cpuinfo[i].state)) == NULL ||
> > PyTuple_SetItem(info, 1, item) < 0)
> > -goto itemError;
> > +goto cleanup;
> >
> > if ((item = libvirt_ulonglongWrap(cpuinfo[i].cpuTime)) == NULL ||
> > PyTuple_SetItem(info, 2, item) < 0)
> > -goto itemError;
> > +goto cleanup;
> >
> > if ((item = libvirt_intWrap((long)cpuinfo[i].cpu)) == NULL ||
> > PyTuple_SetItem(info, 3, item) < 0)
> > -goto itemError;
> > -
> > -if (PyList_SetItem(pycpuinfo, i, info) < 0)
> > -goto itemError;
> > -
> > -continue;
> > - itemError:
> > -Py_DECREF(info);
> > -Py_XDECREF(item);
> > -goto cleanup;
> > +goto cleanup;
> > }
> > for (i = 0; i < dominfo.nrVirtCpu; i++) {
> > PyObject *info = PyTuple_New(cpunum);
> > size_t j;
> > if (info == NULL)
> > goto cleanup;
> > +
> > +if (PyList_SetItem(pycpumap, i, info) < 0)
> > +goto cleanup;
> > +
> > for (j = 0; j < cpunum; j++) {
> > PyObject *item = NULL;
> > if ((item = PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen,
> > i, j))) == NULL ||
> > -PyTuple_SetItem(info, j, item) < 0) {
> > -Py_DECREF(info);
> > -Py_XDECREF(item);
> > +PyTuple_SetItem(info, j, item) < 0)
> > goto cleanup;
> > -}
> > -}
> > -if (PyList_SetItem(pycpumap, i, info) < 0) {
> > -Py_DECREF(info);
> > -goto cleanup;
> > }
> > }
> > -if (PyTuple_SetItem(pyretval, 0, pycpuinfo) < 0 ||
> > -PyTuple_SetItem(pyretval, 1, pycpumap) < 0)
> > -goto cleanup;
> >
> > VIR_FREE(cpuinfo);
> > VIR_FREE(cpumap);
> > @@ -1306,8 +1301,6 @@ libvirt_virDomainGetVcpus(PyObject *self
> > ATTRIBUTE_UNUSED,
> > VIR_FREE(cpuinfo);
> > VIR_FREE(cpumap);
> > Py_XDECREF(pyretval);
> > -Py_XDECREF(pycpuinfo);
> > -Py_XDECREF(pycpumap);
> > return error;
> > }
>
> Splatter from bleeding eyeballs - perhaps would have been easier to read
> with incremental change ;-)
>
> >
> > @@ -1489,12 +1482,13 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self
> > ATTRIBUTE_UNUSED,
> > if (mapinfo == NULL)
> > goto cleanup;
> >
> > +PyList_SetItem(pycpumaps, vcpu, mapinfo);
> > +
> > for (pcpu = 0; pcpu < cpunum; pcpu++) {
> > PyTuple_SetItem(mapinfo, pcpu,
> >
Re: [libvirt] [PATCH] qemuDomainAttachDeviceLive: Check provided disk address
On 25.09.2015 14:45, Martin Kletzander wrote: > On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote: >> >> >> On 09/25/2015 05:38 AM, Michal Privoznik wrote: >>> On 25.09.2015 11:36, Martin Kletzander wrote: On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1257844 > > Imagine an user who is trying to attach a disk to a domain with > the following XML: > > > > > > > > > The XML is obviously wrong. It's trying to attach a virtio disk > onto non-PCI bus. We should forbid that. > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_hotplug.c | 7 +++ > 1 file changed, 7 insertions(+) > How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots(). >>> >>> Exactly! qemuAssignDevicePCISlots() is called only in case of --config. >>> >> >> Seems to me this may be more of a generic problem - a user providing the >> wrong address type for the type of device. I have a recollection of > > Yes, and since we have checks for those, it's confusing to me why > would qemuAssignDevicePCISlots() be called only with --config. Can we > use that code which checks for more things already? For example, the > here-missing virtio-mmio. Yes and no. qemuDomainAssignAddresses() expects to see vm->def which already has the device attached. Moreover, it not only works over all devices in the domain (filling in all the missing addresses), but possibly creating new controllers too (e.g. plugging new pci bridges if running out of addresses on a bus). Does anybody have a bright idea how this can be fixed apart from obvious one - breaking whole address assignment code into parts and reassembling it back together again? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck v2 5/5] 240-no-arp-spoofing.t: get virt-builder install dsniff
We can't be sure the guest will have yum installed as package manager.
Thus delegate the dsniff package install to virt-builder as it provides
an abstraction for that.
---
lib/Sys/Virt/TCK.pm| 2 +-
scripts/nwfilter/240-no-arp-spoofing.t | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index f6fce93..5d9378d 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -405,7 +405,7 @@ sub create_virt_builder_disk {
}
print "# running virt-builder $osname\n";
-`virt-builder --root-password 'password:$password' --output $target
$osname`;
+`virt-builder --install dsniff --root-password 'password:$password'
--output $target $osname`;
return $target;
}
diff --git a/scripts/nwfilter/240-no-arp-spoofing.t
b/scripts/nwfilter/240-no-arp-spoofing.t
index 6b346aa..284033d 100644
--- a/scripts/nwfilter/240-no-arp-spoofing.t
+++ b/scripts/nwfilter/240-no-arp-spoofing.t
@@ -92,7 +92,6 @@ $ssh->login("root", $tck->root_password());
# now generate a arp spoofing packets
diag "generate arpspoof";
my $cmdfile = < /test.sh
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck v2 4/5] Make sure the image has been started properly started the first time.
openSUSE images have a special first boot generating initrd and grub.cfg
according to get rid of the generic one. To ensure this runs properly,
force a first boot right after having generated the disk image.
---
lib/Sys/Virt/TCK.pm | 37 +
1 file changed, 37 insertions(+)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index 04bdbf4..f6fce93 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -738,6 +738,7 @@ sub get_image {
my $bucket = "os-$arch-$ostype";
+my $needs_firstboot = ! $self->has_disk_image($bucket, "disk-$osname.img",
$osname);
my $dfile = $self->create_virt_builder_disk($bucket, "disk-$osname.img",
$osname);
my $dev = $self->get_disk_dev($ostype, $domain);
@@ -750,6 +751,7 @@ sub get_image {
loader => $loader,
root => $dfile,
dev => $dev,
+firstboot => $needs_firstboot,
);
}
@@ -785,6 +787,41 @@ sub generic_machine_domain {
$b->disk(src => $config{root},
dst => $config{dev},
type => "file");
+
+if ($config{firstboot}) {
+print "# Running the first boot\n";
+
+$b->interface(type => "network",
+ source => "default",
+ mac => "52:54:00:11:11:11",
+ filterref => "clean-traffic");
+my $xml = $b->as_xml();
+# Cleanup the temporary interface
+$b->rminterface();
+
+my $dom = $self->conn->define_domain($xml);
+$dom->create();
+
+# Wait for the first boot to reach network setting
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Safe to shutdown domain now
+my $target = time() + 30;
+$dom->shutdown;
+while ($dom->is_active()) {
+ sleep(1);
+ $dom->destroy() if time() > $target;
+}
+sleep(1);
+$dom->undefine();
+}
+
return $b;
} else {
my %config = $self->get_kernel($caps, $ostype);
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck v2 3/5] Add DomainBuilder->rminterface function.
---
lib/Sys/Virt/TCK/DomainBuilder.pm | 7 +++
1 file changed, 7 insertions(+)
diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm
b/lib/Sys/Virt/TCK/DomainBuilder.pm
index 7a20008..e9453ee 100644
--- a/lib/Sys/Virt/TCK/DomainBuilder.pm
+++ b/lib/Sys/Virt/TCK/DomainBuilder.pm
@@ -271,6 +271,13 @@ sub disk {
return $self;
}
+
+sub rminterface {
+my $self = shift;
+
+return pop @{$self->{interfaces}};
+}
+
sub interface {
my $self = shift;
my %params = @_;
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck v2 2/5] Create has_disk_image from code of get_virt_builder_disk
This function will later be reused to check if a first boot is needed.
---
lib/Sys/Virt/TCK.pm | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index a3d06a2..04bdbf4 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -375,6 +375,19 @@ sub create_sparse_disk {
}
+sub has_disk_image {
+my $self = shift;
+my $bucket = shift;
+my $name = shift;
+my $osname = shift;
+
+my $dir = $self->bucket_dir($bucket);
+
+my $target = catfile($dir, $name);
+
+return -f $target
+}
+
sub create_virt_builder_disk {
my $self = shift;
my $bucket = shift;
@@ -387,7 +400,7 @@ sub create_virt_builder_disk {
my $password = $self->root_password;
-if (-f $target) {
+if ($self->has_disk_image($bucket, $name, $osname)) {
return $target;
}
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck v2 0/5] Getting the TCK to run against opensuse images
Hi all, Here are a few patches to get the TCK run properly against opensuse image. The patch adding the opensuse images to virt-builder is still pending, but the main issues were: * Waiting for 30 seconds for the guest to be started is pretty fragile. For the tests pinging the guest, wait for domifstats to report 10 sent packets. * The first boot of the opensuse image runs dracut and takes quite a long time. We need to wait for the first boot to complete before shutting down the guest to avoid inconsistent states. * Yum isn't in all distros by default you know ;) diff to v1: * set the proper email address Cédric Bosdonnat (5): Ping test: use domifstats to wait for dhcp to be setup in guest Create has_disk_image from code of get_virt_builder_disk Add DomainBuilder->rminterface function. Make sure the image has been started properly started the first time. 240-no-arp-spoofing.t: get virt-builder install dsniff lib/Sys/Virt/TCK.pm | 54 +-- lib/Sys/Virt/TCK/DomainBuilder.pm | 7 scripts/nwfilter/100-ping-still-working.t | 13 ++-- scripts/nwfilter/210-no-mac-spoofing.t| 13 ++-- scripts/nwfilter/230-no-mac-broadcast.t | 13 ++-- scripts/nwfilter/240-no-arp-spoofing.t| 14 ++-- 6 files changed, 103 insertions(+), 11 deletions(-) -- 2.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck v2 1/5] Ping test: use domifstats to wait for dhcp to be setup in guest
Some guests may be slower to start than 30 seconds. Use domifstats
to wait for 10 packets to be sent by the guest (usually the DHCP ones
are the first ones).
---
scripts/nwfilter/100-ping-still-working.t | 13 +++--
scripts/nwfilter/210-no-mac-spoofing.t| 13 +++--
scripts/nwfilter/230-no-mac-broadcast.t | 13 +++--
scripts/nwfilter/240-no-arp-spoofing.t| 13 +++--
4 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/scripts/nwfilter/100-ping-still-working.t
b/scripts/nwfilter/100-ping-still-working.t
index f65bf37..a20b95d 100644
--- a/scripts/nwfilter/100-ping-still-working.t
+++ b/scripts/nwfilter/100-ping-still-working.t
@@ -54,8 +54,17 @@ diag "Start domain";
$dom->create;
ok($dom->get_id() > 0, "running domain has an ID > 0");
-diag "Waiting 30 seconds for guest to finish booting";
-sleep(30);
+diag "Waiting for guest to finish booting";
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Wait a little bit more to make sure dhcp is started in the guest
+sleep(10);
my $mac = get_first_macaddress($dom);
diag "mac is $mac";
diff --git a/scripts/nwfilter/210-no-mac-spoofing.t
b/scripts/nwfilter/210-no-mac-spoofing.t
index 7e80216..b81fc4a 100644
--- a/scripts/nwfilter/210-no-mac-spoofing.t
+++ b/scripts/nwfilter/210-no-mac-spoofing.t
@@ -53,8 +53,17 @@ diag "Start domain";
$dom->create;
ok($dom->get_id() > 0, "running domain has an ID > 0");
-diag "Waiting 30 seconds for guest to finish booting";
-sleep(30);
+diag "Waiting for guest to finish booting";
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Wait a little bit more to make sure dhcp is started in the guest
+sleep(10);
# ping guest first nic
my $mac = get_first_macaddress($dom);
diff --git a/scripts/nwfilter/230-no-mac-broadcast.t
b/scripts/nwfilter/230-no-mac-broadcast.t
index 2e616d6..16ce60d 100644
--- a/scripts/nwfilter/230-no-mac-broadcast.t
+++ b/scripts/nwfilter/230-no-mac-broadcast.t
@@ -52,8 +52,17 @@ diag "Start domain";
$dom->create;
ok($dom->get_id() > 0, "running domain has an ID > 0");
-diag "Waiting 30 seconds for guest to finish booting";
-sleep(30);
+diag "Waiting for guest to finish booting";
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Wait a little bit more to make sure dhcp is started in the guest
+sleep(10);
# ping guest first nic
my $mac = get_first_macaddress($dom);
diff --git a/scripts/nwfilter/240-no-arp-spoofing.t
b/scripts/nwfilter/240-no-arp-spoofing.t
index 596a0ce..6b346aa 100644
--- a/scripts/nwfilter/240-no-arp-spoofing.t
+++ b/scripts/nwfilter/240-no-arp-spoofing.t
@@ -54,8 +54,17 @@ diag "Start domain";
$dom->create;
ok($dom->get_id() > 0, "running domain has an ID > 0");
-diag "Waiting 30 seconds for guest to finish booting";
-sleep(30);
+diag "Waiting for guest to finish booting";
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Wait a little bit more to make sure dhcp is started in the guest
+sleep(10);
# ping guest first nic
my $mac = get_first_macaddress($dom);
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-python PATCH 15/23] all Py*_New function has to be checked for return value
On Sat, Sep 26, 2015 at 09:37:23AM -0400, John Ferlan wrote:
>
> $subj:
>
> Must check return value for all Py*_New functions
Thanks, I'll update it.
>
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > If the function fails, we need to cleanup memory and return NULL.
> >
> > Signed-off-by: Pavel Hrdina
> > ---
> > libvirt-lxc-override.c | 10 +-
> > libvirt-override.c | 269
> > +
> > 2 files changed, 190 insertions(+), 89 deletions(-)
> >
>
> General comment... 4 patches ago we were returning PyErr_NoMemory on
> allocation failures - that would seem to be the case for these too,
> right? Or is this "just different" enough that it would be necessary?
No, because all python APIs sets an exception in case of error.
>
> > diff --git a/libvirt-lxc-override.c b/libvirt-lxc-override.c
> > index 20d1cf4..b0550c7 100644
> > --- a/libvirt-lxc-override.c
> > +++ b/libvirt-lxc-override.c
> > @@ -79,7 +79,9 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self
> > ATTRIBUTE_UNUSED,
> > if (c_retval < 0)
> > return VIR_PY_NONE;
> >
> > -py_retval = PyList_New(0);
> > +if ((py_retval = PyList_New(0)) == NULL)
> > +goto error;
> > +
> > for (i = 0; i < c_retval; i++) {
> > PyObject *item = NULL;
> >
> > @@ -91,6 +93,8 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self
> > ATTRIBUTE_UNUSED,
> > goto error;
> > }
> > }
> > +
> > + cleanup:
> > VIR_FREE(fdlist);
> > return py_retval;
> >
> > @@ -98,8 +102,8 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self
> > ATTRIBUTE_UNUSED,
> > for (i = 0; i < c_retval; i++) {
> > VIR_FORCE_CLOSE(fdlist[i]);
> > }
> > -VIR_FREE(fdlist);
> > -return NULL;
> > +Py_CLEAR(py_retval);
> > +goto cleanup;
> > }
> > /
> > * *
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 0df6844..92c31b8 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -1859,7 +1859,7 @@ static void
> > libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx,
> > virErrorPtr err)
> > {
> > -PyObject *list, *info;
> > +PyObject *list = NULL, *info = NULL;
> > PyObject *result;
> >
> > DEBUG("libvirt_virErrorFuncHandler(%p, %s, ...) called\n", ctx,
> > @@ -1874,8 +1874,12 @@ libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void
> > *ctx,
> > (libvirt_virPythonErrorFuncHandler == Py_None)) {
> > virDefaultErrorFunc(err);
> > } else {
> > -list = PyTuple_New(2);
> > -info = PyTuple_New(9);
> > +if ((list = PyTuple_New(2)) == NULL)
> > +goto cleanup;
> > +
> > +if ((info = PyTuple_New(9)) == NULL)
> > +goto cleanup;
> > +
> > PyTuple_SetItem(list, 0, libvirt_virPythonErrorFuncCtxt);
> > PyTuple_SetItem(list, 1, info);
> > Py_XINCREF(libvirt_virPythonErrorFuncCtxt);
> > @@ -1894,6 +1898,10 @@ libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void
> > *ctx,
> > Py_XDECREF(result);
> > }
> >
> > + cleanup:
> > +Py_XDECREF(list);
> > +Py_XDECREF(info);
> > +
> > LIBVIRT_RELEASE_THREAD_STATE;
> > }
> >
> > @@ -1938,12 +1946,12 @@
> > virConnectCredCallbackWrapper(virConnectCredentialPtr cred,
> >unsigned int ncred,
> >void *cbdata)
> > {
> > -PyObject *list;
> > +PyObject *list = NULL;
> > PyObject *pycred;
> > PyObject *pyauth = (PyObject *)cbdata;
> > PyObject *pycbdata;
> > PyObject *pycb;
> > -PyObject *pyret;
> > +PyObject *pyret = NULL;
> > int ret = -1;
> > size_t i;
> >
> > @@ -1952,11 +1960,17 @@
> > virConnectCredCallbackWrapper(virConnectCredentialPtr cred,
> > pycb = PyList_GetItem(pyauth, 1);
> > pycbdata = PyList_GetItem(pyauth, 2);
> >
> > -list = PyTuple_New(2);
> > -pycred = PyTuple_New(ncred);
> > +if((list = PyTuple_New(2)) == NULL)
>
> if ((
Nice catch, thanks.
>
> > +goto cleanup;
> > +
> > +if ((pycred = PyTuple_New(ncred)) == NULL)
> > +goto cleanup;
> > +
> > for (i = 0; i < ncred; i++) {
> > -PyObject *pycreditem;
> > -pycreditem = PyList_New(5);
> > +PyObject *pycreditem = PyList_New(5);
> > +if (pycreditem == NULL)
> > +goto cleanup;
> > +
>
> nit: could all be one line (any typos are mine ;-))
>
>if ((pycreditem = PyList_New(5)) == NULL)
>
> Nothing jumped out at me in the rest - although I admit my eyes & brain
> are starting to tire ;-)
It's a lot of almost same changes, easy to miss something. I'll update that,
thanks.
Pavel
>
> John
> > PyTuple_SetItem(pycred, i, pycreditem);
> > PyList_SetItem(pycreditem, 0,
Re: [libvirt] [PATCH tck 1/5] Ping test: use domifstats to wait for dhcp to be setup in guest
On Tue, 2015-09-29 at 16:48 +0200, Cédric Bosdonnat wrote:
> From: Cédric Bosdonnat
Ouch, the email address is the wrong one here. I'll resubmit the series
with the correct one.
--
Cedric
> Some guests may be slower to start than 30 seconds. Use domifstats
> to wait for 10 packets to be sent by the guest (usually the DHCP ones
> are the first ones).
> ---
> scripts/nwfilter/100-ping-still-working.t | 13 +++--
> scripts/nwfilter/210-no-mac-spoofing.t| 13 +++--
> scripts/nwfilter/230-no-mac-broadcast.t | 13 +++--
> scripts/nwfilter/240-no-arp-spoofing.t| 13 +++--
> 4 files changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/nwfilter/100-ping-still-working.t
> b/scripts/nwfilter/100-ping-still-working.t
> index f65bf37..a20b95d 100644
> --- a/scripts/nwfilter/100-ping-still-working.t
> +++ b/scripts/nwfilter/100-ping-still-working.t
> @@ -54,8 +54,17 @@ diag "Start domain";
> $dom->create;
> ok($dom->get_id() > 0, "running domain has an ID > 0");
>
> -diag "Waiting 30 seconds for guest to finish booting";
> -sleep(30);
> +diag "Waiting for guest to finish booting";
> +my $stats;
> +my $tries = 0;
> +do {
> +sleep(10);
> +$stats = $dom->interface_stats("vnet0");
> +$tries++;
> +} while ($stats->{"tx_packets"} < 10 && $tries < 10);
> +
> +# Wait a little bit more to make sure dhcp is started in the guest
> +sleep(10);
>
> my $mac = get_first_macaddress($dom);
> diag "mac is $mac";
> diff --git a/scripts/nwfilter/210-no-mac-spoofing.t
> b/scripts/nwfilter/210-no-mac-spoofing.t
> index 7e80216..b81fc4a 100644
> --- a/scripts/nwfilter/210-no-mac-spoofing.t
> +++ b/scripts/nwfilter/210-no-mac-spoofing.t
> @@ -53,8 +53,17 @@ diag "Start domain";
> $dom->create;
> ok($dom->get_id() > 0, "running domain has an ID > 0");
>
> -diag "Waiting 30 seconds for guest to finish booting";
> -sleep(30);
> +diag "Waiting for guest to finish booting";
> +my $stats;
> +my $tries = 0;
> +do {
> +sleep(10);
> +$stats = $dom->interface_stats("vnet0");
> +$tries++;
> +} while ($stats->{"tx_packets"} < 10 && $tries < 10);
> +
> +# Wait a little bit more to make sure dhcp is started in the guest
> +sleep(10);
>
> # ping guest first nic
> my $mac = get_first_macaddress($dom);
> diff --git a/scripts/nwfilter/230-no-mac-broadcast.t
> b/scripts/nwfilter/230-no-mac-broadcast.t
> index 2e616d6..16ce60d 100644
> --- a/scripts/nwfilter/230-no-mac-broadcast.t
> +++ b/scripts/nwfilter/230-no-mac-broadcast.t
> @@ -52,8 +52,17 @@ diag "Start domain";
> $dom->create;
> ok($dom->get_id() > 0, "running domain has an ID > 0");
>
> -diag "Waiting 30 seconds for guest to finish booting";
> -sleep(30);
> +diag "Waiting for guest to finish booting";
> +my $stats;
> +my $tries = 0;
> +do {
> +sleep(10);
> +$stats = $dom->interface_stats("vnet0");
> +$tries++;
> +} while ($stats->{"tx_packets"} < 10 && $tries < 10);
> +
> +# Wait a little bit more to make sure dhcp is started in the guest
> +sleep(10);
>
> # ping guest first nic
> my $mac = get_first_macaddress($dom);
> diff --git a/scripts/nwfilter/240-no-arp-spoofing.t
> b/scripts/nwfilter/240-no-arp-spoofing.t
> index 596a0ce..6b346aa 100644
> --- a/scripts/nwfilter/240-no-arp-spoofing.t
> +++ b/scripts/nwfilter/240-no-arp-spoofing.t
> @@ -54,8 +54,17 @@ diag "Start domain";
> $dom->create;
> ok($dom->get_id() > 0, "running domain has an ID > 0");
>
> -diag "Waiting 30 seconds for guest to finish booting";
> -sleep(30);
> +diag "Waiting for guest to finish booting";
> +my $stats;
> +my $tries = 0;
> +do {
> +sleep(10);
> +$stats = $dom->interface_stats("vnet0");
> +$tries++;
> +} while ($stats->{"tx_packets"} < 10 && $tries < 10);
> +
> +# Wait a little bit more to make sure dhcp is started in the guest
> +sleep(10);
>
> # ping guest first nic
> my $mac = get_first_macaddress($dom);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck 5/5] 240-no-arp-spoofing.t: get virt-builder install dsniff
From: Cédric Bosdonnat
We can't be sure the guest will have yum installed as package manager.
Thus delegate the dsniff package install to virt-builder as it provides
an abstraction for that.
---
lib/Sys/Virt/TCK.pm| 2 +-
scripts/nwfilter/240-no-arp-spoofing.t | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index f6fce93..5d9378d 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -405,7 +405,7 @@ sub create_virt_builder_disk {
}
print "# running virt-builder $osname\n";
-`virt-builder --root-password 'password:$password' --output $target
$osname`;
+`virt-builder --install dsniff --root-password 'password:$password'
--output $target $osname`;
return $target;
}
diff --git a/scripts/nwfilter/240-no-arp-spoofing.t
b/scripts/nwfilter/240-no-arp-spoofing.t
index 6b346aa..284033d 100644
--- a/scripts/nwfilter/240-no-arp-spoofing.t
+++ b/scripts/nwfilter/240-no-arp-spoofing.t
@@ -92,7 +92,6 @@ $ssh->login("root", $tck->root_password());
# now generate a arp spoofing packets
diag "generate arpspoof";
my $cmdfile = < /test.sh
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck 4/5] Make sure the image has been started properly started the first time.
From: Cédric Bosdonnat
openSUSE images have a special first boot generating initrd and grub.cfg
according to get rid of the generic one. To ensure this runs properly,
force a first boot right after having generated the disk image.
---
lib/Sys/Virt/TCK.pm | 37 +
1 file changed, 37 insertions(+)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index 04bdbf4..f6fce93 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -738,6 +738,7 @@ sub get_image {
my $bucket = "os-$arch-$ostype";
+my $needs_firstboot = ! $self->has_disk_image($bucket, "disk-$osname.img",
$osname);
my $dfile = $self->create_virt_builder_disk($bucket, "disk-$osname.img",
$osname);
my $dev = $self->get_disk_dev($ostype, $domain);
@@ -750,6 +751,7 @@ sub get_image {
loader => $loader,
root => $dfile,
dev => $dev,
+firstboot => $needs_firstboot,
);
}
@@ -785,6 +787,41 @@ sub generic_machine_domain {
$b->disk(src => $config{root},
dst => $config{dev},
type => "file");
+
+if ($config{firstboot}) {
+print "# Running the first boot\n";
+
+$b->interface(type => "network",
+ source => "default",
+ mac => "52:54:00:11:11:11",
+ filterref => "clean-traffic");
+my $xml = $b->as_xml();
+# Cleanup the temporary interface
+$b->rminterface();
+
+my $dom = $self->conn->define_domain($xml);
+$dom->create();
+
+# Wait for the first boot to reach network setting
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Safe to shutdown domain now
+my $target = time() + 30;
+$dom->shutdown;
+while ($dom->is_active()) {
+ sleep(1);
+ $dom->destroy() if time() > $target;
+}
+sleep(1);
+$dom->undefine();
+}
+
return $b;
} else {
my %config = $self->get_kernel($caps, $ostype);
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck 3/5] Add DomainBuilder->rminterface function.
From: Cédric Bosdonnat
---
lib/Sys/Virt/TCK/DomainBuilder.pm | 7 +++
1 file changed, 7 insertions(+)
diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm
b/lib/Sys/Virt/TCK/DomainBuilder.pm
index 7a20008..e9453ee 100644
--- a/lib/Sys/Virt/TCK/DomainBuilder.pm
+++ b/lib/Sys/Virt/TCK/DomainBuilder.pm
@@ -271,6 +271,13 @@ sub disk {
return $self;
}
+
+sub rminterface {
+my $self = shift;
+
+return pop @{$self->{interfaces}};
+}
+
sub interface {
my $self = shift;
my %params = @_;
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck 2/5] Create has_disk_image from code of get_virt_builder_disk
From: Cédric Bosdonnat
This function will later be reused to check if a first boot is needed.
---
lib/Sys/Virt/TCK.pm | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index a3d06a2..04bdbf4 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -375,6 +375,19 @@ sub create_sparse_disk {
}
+sub has_disk_image {
+my $self = shift;
+my $bucket = shift;
+my $name = shift;
+my $osname = shift;
+
+my $dir = $self->bucket_dir($bucket);
+
+my $target = catfile($dir, $name);
+
+return -f $target
+}
+
sub create_virt_builder_disk {
my $self = shift;
my $bucket = shift;
@@ -387,7 +400,7 @@ sub create_virt_builder_disk {
my $password = $self->root_password;
-if (-f $target) {
+if ($self->has_disk_image($bucket, $name, $osname)) {
return $target;
}
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck 1/5] Ping test: use domifstats to wait for dhcp to be setup in guest
From: Cédric Bosdonnat
Some guests may be slower to start than 30 seconds. Use domifstats
to wait for 10 packets to be sent by the guest (usually the DHCP ones
are the first ones).
---
scripts/nwfilter/100-ping-still-working.t | 13 +++--
scripts/nwfilter/210-no-mac-spoofing.t| 13 +++--
scripts/nwfilter/230-no-mac-broadcast.t | 13 +++--
scripts/nwfilter/240-no-arp-spoofing.t| 13 +++--
4 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/scripts/nwfilter/100-ping-still-working.t
b/scripts/nwfilter/100-ping-still-working.t
index f65bf37..a20b95d 100644
--- a/scripts/nwfilter/100-ping-still-working.t
+++ b/scripts/nwfilter/100-ping-still-working.t
@@ -54,8 +54,17 @@ diag "Start domain";
$dom->create;
ok($dom->get_id() > 0, "running domain has an ID > 0");
-diag "Waiting 30 seconds for guest to finish booting";
-sleep(30);
+diag "Waiting for guest to finish booting";
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Wait a little bit more to make sure dhcp is started in the guest
+sleep(10);
my $mac = get_first_macaddress($dom);
diag "mac is $mac";
diff --git a/scripts/nwfilter/210-no-mac-spoofing.t
b/scripts/nwfilter/210-no-mac-spoofing.t
index 7e80216..b81fc4a 100644
--- a/scripts/nwfilter/210-no-mac-spoofing.t
+++ b/scripts/nwfilter/210-no-mac-spoofing.t
@@ -53,8 +53,17 @@ diag "Start domain";
$dom->create;
ok($dom->get_id() > 0, "running domain has an ID > 0");
-diag "Waiting 30 seconds for guest to finish booting";
-sleep(30);
+diag "Waiting for guest to finish booting";
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Wait a little bit more to make sure dhcp is started in the guest
+sleep(10);
# ping guest first nic
my $mac = get_first_macaddress($dom);
diff --git a/scripts/nwfilter/230-no-mac-broadcast.t
b/scripts/nwfilter/230-no-mac-broadcast.t
index 2e616d6..16ce60d 100644
--- a/scripts/nwfilter/230-no-mac-broadcast.t
+++ b/scripts/nwfilter/230-no-mac-broadcast.t
@@ -52,8 +52,17 @@ diag "Start domain";
$dom->create;
ok($dom->get_id() > 0, "running domain has an ID > 0");
-diag "Waiting 30 seconds for guest to finish booting";
-sleep(30);
+diag "Waiting for guest to finish booting";
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Wait a little bit more to make sure dhcp is started in the guest
+sleep(10);
# ping guest first nic
my $mac = get_first_macaddress($dom);
diff --git a/scripts/nwfilter/240-no-arp-spoofing.t
b/scripts/nwfilter/240-no-arp-spoofing.t
index 596a0ce..6b346aa 100644
--- a/scripts/nwfilter/240-no-arp-spoofing.t
+++ b/scripts/nwfilter/240-no-arp-spoofing.t
@@ -54,8 +54,17 @@ diag "Start domain";
$dom->create;
ok($dom->get_id() > 0, "running domain has an ID > 0");
-diag "Waiting 30 seconds for guest to finish booting";
-sleep(30);
+diag "Waiting for guest to finish booting";
+my $stats;
+my $tries = 0;
+do {
+sleep(10);
+$stats = $dom->interface_stats("vnet0");
+$tries++;
+} while ($stats->{"tx_packets"} < 10 && $tries < 10);
+
+# Wait a little bit more to make sure dhcp is started in the guest
+sleep(10);
# ping guest first nic
my $mac = get_first_macaddress($dom);
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH tck 0/5] Getting the TCK to run against opensuse images
Hi all, Here are a few patches to get the TCK run properly against opensuse image. The patch adding the opensuse images to virt-builder is still pending, but the main issues were: * Waiting for 30 seconds for the guest to be started is pretty fragile. For the tests pinging the guest, wait for domifstats to report 10 sent packets. * The first boot of the opensuse image runs dracut and takes quite a long time. We need to wait for the first boot to complete before shutting down the guest to avoid inconsistent states. * Yum isn't in all distros by default you know ;) Cédric Bosdonnat (5): Ping test: use domifstats to wait for dhcp to be setup in guest Create has_disk_image from code of get_virt_builder_disk Add DomainBuilder->rminterface function. Make sure the image has been started properly started the first time. 240-no-arp-spoofing.t: get virt-builder install dsniff lib/Sys/Virt/TCK.pm | 54 +-- lib/Sys/Virt/TCK/DomainBuilder.pm | 7 scripts/nwfilter/100-ping-still-working.t | 13 ++-- scripts/nwfilter/210-no-mac-spoofing.t| 13 ++-- scripts/nwfilter/230-no-mac-broadcast.t | 13 ++-- scripts/nwfilter/240-no-arp-spoofing.t| 14 ++-- 6 files changed, 103 insertions(+), 11 deletions(-) -- 2.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-python PATCH 13/23] Use VIR_PY_NONE instead
On Sat, Sep 26, 2015 at 09:24:30AM -0400, John Ferlan wrote:
>
> in the subject "instead of Py_None"
>
> and perhaps a few more details in the commit message regarding what's
> being done and why
Thanks, I'll update the commit message.
>
>
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina
> > ---
> > libvirt-override.c | 9 +++--
> > typewrappers.c | 54
> > ++
> > 2 files changed, 21 insertions(+), 42 deletions(-)
> >
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index d2279de..4d0fb92 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -1960,7 +1960,6 @@ virConnectCredCallbackWrapper(virConnectCredentialPtr
> > cred,
> > for (i = 0; i < ncred; i++) {
> > PyObject *pycreditem;
> > pycreditem = PyList_New(5);
> > -Py_INCREF(Py_None);
>
> I'm curious as to why it was done before and why it's OK to remove it.
> The others mostly make sense - this functions usage is, well, odd
This first increment of Py_None is used there [1] ...
>
> John
> > PyTuple_SetItem(pycred, i, pycreditem);
> > PyList_SetItem(pycreditem, 0, libvirt_intWrap((long)
> > cred[i].type));
> > PyList_SetItem(pycreditem, 1,
> > libvirt_constcharPtrWrap(cred[i].prompt));
> > @@ -1968,17 +1967,15 @@
> > virConnectCredCallbackWrapper(virConnectCredentialPtr cred,
> > PyList_SetItem(pycreditem, 2,
> > libvirt_constcharPtrWrap(cred[i].challenge));
> > } else {
> > -Py_INCREF(Py_None);
> > -PyList_SetItem(pycreditem, 2, Py_None);
> > +PyList_SetItem(pycreditem, 2, VIR_PY_NONE);
> > }
> > if (cred[i].defresult) {
> > PyList_SetItem(pycreditem, 3,
> > libvirt_constcharPtrWrap(cred[i].defresult));
> > } else {
> > -Py_INCREF(Py_None);
> > -PyList_SetItem(pycreditem, 3, Py_None);
> > +PyList_SetItem(pycreditem, 3, VIR_PY_NONE);
> > }
> > -PyList_SetItem(pycreditem, 4, Py_None);
> > +PyList_SetItem(pycreditem, 4, VIR_PY_NONE);
[1], the other increments are directly before using the Py_None object.
Pavel
> > }
> >
> > PyTuple_SetItem(list, 0, pycred);
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-python PATCH 03/23] remove useless check for NULL before Py_XDECREF
On Sat, Sep 26, 2015 at 09:08:35AM -0400, John Ferlan wrote:
>
>
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina
> > ---
> > libvirt-override.c | 9 +++--
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 14aa0e9..114104b 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -2303,12 +2303,9 @@ libvirt_virRegisterErrorHandler(ATTRIBUTE_UNUSED
> > PyObject * self,
> >pyobj_f);
> >
> > virSetErrorFunc(NULL, libvirt_virErrorFuncHandler);
> > -if (libvirt_virPythonErrorFuncHandler != NULL) {
> > -Py_XDECREF(libvirt_virPythonErrorFuncHandler);
> > -}
> > -if (libvirt_virPythonErrorFuncCtxt != NULL) {
> > -Py_XDECREF(libvirt_virPythonErrorFuncCtxt);
> > -}
> > +
> > +Py_XDECREF(libvirt_virPythonErrorFuncHandler);
> > +Py_XDECREF(libvirt_virPythonErrorFuncCtxt);
>
> I keep looking at this and thinking why? Why was it added and what is
> it protecting. Looking at libvirt_virErrorFuncHandler it seems only
> libvirt_virPythonErrorFuncCtxt gets the Py_XINCREF and that's based on
> whether libvirt_virPythonErrorFuncHandler is NULL or not.
I don't understand what do you mean by this??
>
> Just feels like something subtle is going on. Also it's a register
> handler and we're decrementing something that never got incremented.
Check the whole function. It will decrement the old handlers, check for
existence of new handlers and clean/set new handlers. And before setting the
new handlers, we need to increment them, otherwise they will be cleared by GC.
Everything that is parsed by 'PyArg_ParseTuple' has to be incremented, if you
need to use those object outside of the function scope, where you've called that
parser.
Pavel
>
> John
> >
> > if ((pyobj_f == Py_None) && (pyobj_ctx == Py_None)) {
> > libvirt_virPythonErrorFuncHandler = NULL;
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-python PATCH 02/23] refactor the function to not override python exceptions
On Sat, Sep 26, 2015 at 09:08:11AM -0400, John Ferlan wrote:
>
>
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina
> > ---
> > libvirt-override.c | 23 ---
> > 1 file changed, 8 insertions(+), 15 deletions(-)
> >
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 4dfe332..14aa0e9 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -8263,38 +8263,31 @@ libvirt_virDomainSetTime(PyObject *self
> > ATTRIBUTE_UNUSED, PyObject *args) {
>
> Seems as though py_retval can be removed as it's no longer needed (build
> whine)
>
It seems, that I forgot to remove the declaration of unused variables through
the whole series. Thanks for noticing that, I didn't get any error during
compilation.
>
>
> > if (PyDict_Check(py_dict)) {
> > py_dict_size = PyDict_Size(py_dict);
> > if ((pyobj_seconds = PyDict_GetItemString(py_dict, "seconds"))) {
> > -if (libvirt_longlongUnwrap(pyobj_seconds, &seconds) < 0) {
> > -PyErr_Format(PyExc_LookupError, "malformed 'seconds'");
> > -goto cleanup;
> > -}
> > +if (libvirt_longlongUnwrap(pyobj_seconds, &seconds) < 0)
> > +return NULL;
>
> So if this was missing - would the error indicate 'seconds' was
> malformed? Or just some generic invalid value python error? I'm
> preferential to specific errors that help point me at what I did wrong.
Well, the reason, that the unwrap function fails don't have to always be
relevant to the "malformed 'second'" and it could mislead the user that it's his
fault, but it could be anything else. For example, the unwrap function checks,
whether the pyobj_seconds is number or not.
Pavel
>
> John
> > } else {
> > PyErr_Format(PyExc_LookupError, "Dictionary must contains
> > 'seconds'");
> > -goto cleanup;
> > +return NULL;
> > }
> >
> > if ((pyobj_nseconds = PyDict_GetItemString(py_dict, "nseconds"))) {
> > -if (libvirt_uintUnwrap(pyobj_nseconds, &nseconds) < 0) {
> > -PyErr_Format(PyExc_LookupError, "malformed 'nseconds'");
> > -goto cleanup;
> > -}
> > +if (libvirt_uintUnwrap(pyobj_nseconds, &nseconds) < 0)
> > +return NULL;
> > } else if (py_dict_size > 1) {
> > PyErr_Format(PyExc_LookupError, "Dictionary contains unknown
> > key");
> > -goto cleanup;
> > +return NULL;
> > }
> > } else if (py_dict != Py_None || !flags) {
> > PyErr_Format(PyExc_TypeError, "time must be a dictionary "
> > "or None with flags set");
> > -goto cleanup;
> > +return NULL;
> > }
> >
> > LIBVIRT_BEGIN_ALLOW_THREADS;
> > c_retval = virDomainSetTime(domain, seconds, nseconds, flags);
> > LIBVIRT_END_ALLOW_THREADS;
> >
> > -py_retval = libvirt_intWrap(c_retval);
> > -
> > - cleanup:
> > -return py_retval;
> > +return libvirt_intWrap(c_retval);
> > }
> > #endif /* LIBVIR_CHECK_VERSION(1, 2, 5) */
> >
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Fix dynamic_ownership qemu.conf setting
On 09/29/2015 12:46 AM, Martin Kletzander wrote: > On Mon, Sep 28, 2015 at 07:53:42PM -0400, Cole Robinson wrote: >> Commit 307fb904 (Sep 10) added a 'privileged' variable when creating >> the DAC driver: >> >> @@ -153,6 +157,7 @@ virSecurityManagerNewDAC(const char *virtDriver, >> bool defaultConfined, >> bool requireConfined, >> bool dynamicOwnership, >> + bool privileged, >> virSecurityManagerDACChownCallback chownCallback) >> >> But argument order is mixed up at the caller, swapping dynamicOwnership >> and privileged values. This corrects the argument order >> > > I'm pretty sure I noticed this during review. I must've failed to > inform the author because I see no reply to the patch that caused it. > > ACK and safe for freeze! > Thanks, pushed now - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv3 0/2] Added waiting for DAD to finish for bridge address.
This is a fix for commit db488c79173b240459c7754f38c3c6af9b432970 dnsmasq main process which is relied on when waiting for DAD to complete exits without actually waiting for DAD. This is dnsmasq daemon's task. It seems to be a race that DAD finished before dnsmasq main process exited. The above commit needs the execution to block until DAD finishes for bridge IPv6 address because then it closes dummy tap device. Thus we need to ensure this ourselves. So we periodically poll the kernel using netlink and check whether there are any IPv6 addresses assigned to bridge which have 'tentative' state. After DAD is finished, execution continues. I guess that is what dnsmasq was assumed to do. We use netlink to dump information about existing IPv6 addresses. Netlink's response is a multi-part message. Unfortunately, the current implementation of virNetlink treats such messages as faulty and throws an error. So the patch 2/2 adds multi-part nelink response support. Update v2: fixed syntax. Update v3: moved to virnetdev. Resend: These patches were ignored and buried long ago :-( Maxim Perevedentsev (2): network: added waiting for DAD to finish for bridge address. netlink: add support for multi-part netlink messages. src/libvirt_private.syms| 1 + src/network/bridge_driver.c | 35 +- src/util/virnetdev.c| 160 src/util/virnetdev.h| 2 + src/util/virnetlink.c | 4 +- 5 files changed, 200 insertions(+), 2 deletions(-) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv3 2/2] netlink: add support for multi-part netlink messages.
Such messages do not have NLMSG_ERROR or NLMSG_DONE type but they are valid responses. We test 'multi-partness' by looking for NLM_F_MULTI flag. --- Difference to v1: fixed comment style. src/util/virnetlink.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 0276522..679b48e 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -394,7 +394,9 @@ virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen) break; default: -goto malformed_resp; +/* We allow multipart messages. */ +if (!(resp->nlmsg_flags & NLM_F_MULTI)) +goto malformed_resp; } return result; -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv3 1/2] network: added waiting for DAD to finish for bridge address.
This is a fix for commit db488c79173b240459c7754f38c3c6af9b432970
dnsmasq main process exits without waiting for DAD, this is dnsmasq
daemon's task. So we periodically poll the kernel using netlink and
check whether there are any IPv6 addresses assigned to bridge
which have 'tentative' state. After DAD is finished, execution continues.
I guess that is what dnsmasq was assumed to do.
---
Difference to v1: fixed syntax (comments and alignment).
Difference to v2: moved to virnetdev.
src/libvirt_private.syms| 1 +
src/network/bridge_driver.c | 35 +-
src/util/virnetdev.c| 160
src/util/virnetdev.h| 2 +
4 files changed, 197 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c87efa1..514560b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1791,6 +1791,7 @@ virNetDevSetRcvMulti;
virNetDevSetupControl;
virNetDevSysfsFile;
virNetDevValidateConfig;
+virNetDevWaitDadFinish;
# util/virnetdevbandwidth.h
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c343e5b..0a93678 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2026,6 +2026,33 @@ networkAddRouteToBridge(virNetworkObjPtr network,
}
static int
+networkWaitDadFinish(virNetworkObjPtr network)
+{
+virNetworkIpDefPtr ipdef;
+virSocketAddrPtr *addrs = NULL;
+size_t i;
+int ret;
+for (i = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, i));
+ i++) {}
+
+if (i == 0)
+return 0;
+if (VIR_ALLOC_N(addrs, i))
+return -1;
+
+for (i = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, i));
+ i++) {
+addrs[i] = &ipdef->address;
+}
+
+ret = virNetDevWaitDadFinish(addrs, i);
+VIR_FREE(addrs);
+return ret;
+}
+
+static int
networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
virNetworkObjPtr network)
{
@@ -2159,7 +2186,13 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr
driver,
if (v6present && networkStartRadvd(driver, network) < 0)
goto err4;
-/* DAD has happened (dnsmasq waits for it), dnsmasq is now bound to the
+/* dnsmasq main process does not wait for DAD to complete,
+ * so we need to wait for it ourselves.
+ */
+if (v6present && networkWaitDadFinish(network) < 0)
+goto err4;
+
+/* DAD has happened, dnsmasq is now bound to the
* bridge's IPv6 address, so we can now set the dummy tun down.
*/
if (tapfd >= 0) {
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 579ff3b..3d77b37 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -96,6 +96,7 @@ VIR_LOG_INIT("util.netdev");
# define FEATURE_BIT_IS_SET(blocks, index, field)\
(FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
#endif
+# define IP_BUF_SIZE 4096
typedef enum {
VIR_MCAST_TYPE_INDEX_TOKEN,
@@ -1305,6 +1306,103 @@ int virNetDevClearIPAddress(const char *ifname,
return ret;
}
+/* return whether there is a known address with 'tentative' flag set */
+static int
+virNetDevParseDadStatus(struct nlmsghdr *nlh, int len,
+virSocketAddrPtr *addrs, size_t count)
+{
+struct ifaddrmsg *ifaddrmsg_ptr;
+unsigned int ifaddrmsg_len;
+struct rtattr *rtattr_ptr;
+size_t i;
+struct in6_addr *addr;
+for (; NLMSG_OK(nlh, len); nlh = NLMSG_NEXT(nlh, len)) {
+if (NLMSG_PAYLOAD(nlh, 0) < sizeof(struct ifaddrmsg)) {
+/* Message without payload is the last one. */
+break;
+}
+
+ifaddrmsg_ptr = (struct ifaddrmsg *)NLMSG_DATA(nlh);
+if (!(ifaddrmsg_ptr->ifa_flags & IFA_F_TENTATIVE)) {
+/* Not tentative: we are not interested in this entry. */
+continue;
+}
+
+ifaddrmsg_len = IFA_PAYLOAD(nlh);
+rtattr_ptr = (struct rtattr *) IFA_RTA(ifaddrmsg_ptr);
+for (; RTA_OK(rtattr_ptr, ifaddrmsg_len);
+rtattr_ptr = RTA_NEXT(rtattr_ptr, ifaddrmsg_len)) {
+if (RTA_PAYLOAD(rtattr_ptr) != sizeof(struct in6_addr)) {
+/* No address: ignore. */
+continue;
+}
+
+/* We check only known addresses. */
+for (i = 0; i < count; i++) {
+addr = &addrs[i]->data.inet6.sin6_addr;
+if (!memcmp(addr, RTA_DATA(rtattr_ptr),
+sizeof(struct in6_addr))) {
+/* We found matching tentative address. */
+return 1;
+}
+}
+}
+}
+return 0;
+}
+
+/* return after DAD finishes for all known IPv6 addresses or an error */
+int
+virNetDevWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
+{
+struct nl_msg *nlmsg = NULL;
+struct ifaddrmsg ifa;
+struct nlmsghdr *resp = NULL;
+
Re: [libvirt] [PATCH] conf: Fix virtType check
On Fri, Sep 25, 2015 at 17:39:57 -0400, John Ferlan wrote:
> Commit id '7383b8cc' changed virDomainDef 'virtType' to an enum, that
Sigh. That was the reason we didn't do that yet. Changing stuff to enum
is likely to break many things.
> caused a build failure on some archs due to comparing an unsigned value
> to < 0. Adjust the fetch of 'type' to be into temporary 'int virtType'
> and then assign that virtType to the def->virtType
>
> Signed-off-by: John Ferlan
> ---
> Pushed as build breaker
>
> src/conf/domain_conf.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index cdafb14..393ece7 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14640,7 +14640,7 @@ virDomainDefParseXML(xmlDocPtr xml,
> xmlNodePtr *nodes = NULL, node = NULL;
> char *tmp = NULL;
> size_t i, j;
> -int n;
> +int n, virtType;
Please put them on separate lines.
> long id = -1;
> virDomainDefPtr def;
> bool uuid_generated = false;
> @@ -14678,11 +14678,12 @@ virDomainDefParseXML(xmlDocPtr xml,
> goto error;
> }
>
> -if ((def->virtType = virDomainVirtTypeFromString(tmp)) < 0) {
> +if ((virtType = virDomainVirtTypeFromString(tmp)) < 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("invalid domain type %s"), tmp);
> goto error;
> }
> +def->virtType = virtType;
> VIR_FREE(tmp);
>
> def->os.bootloader = virXPathString("string(./bootloader)", ctxt);
ACK with the above change.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] interface: let netcf pre-filter for active vs. inactive
On Fri, Sep 25, 2015 at 11:13:56AM -0400, Laine Stump wrote: > If a system has a large number of active or active interfaces, it can *active or inactive Jan > be a big waste of time to retrieve and qualify all interfaces if the > caller only wanted one subset. Since netcf has a simple flag for this, > translate the libvirt flag into a netcf flag and let netcf pre-filter. > --- > src/interface/interface_backend_netcf.c | 40 > + > 1 file changed, 16 insertions(+), 24 deletions(-) > signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
