8:47 a.m.
So, this series fixes a few issues that are very tightly coupled, so buckle
up if you want to read further, if not, here you go:
TL;DR:
Issue: there's a test passing even though the code doesn't really work
Result: fix both the code and the test
LONG STORY:
The main issue is that we allow starting a domain with multiple OpenGL-enabled
graphics devices:
<graphics type='spice'>
<listen type='none'/>
<gl enable='yes' rendernode='foo'/>
</graphics>
<graphics type='egl-headless'/>
But hey, we have both a validation code handling this and a test case and
everything seems to work. Well, not really, since the validation code for
graphics was never run because it runs as a callback from our device iterator
which only iterates over devices which provide boot info data which graphics do
not provide. Any attempt to pass some arbitrary boot info data for graphics
will cause troubles in other callbacks that are invoked from within the
iterator. The way to enable it is to introduce some flags to alter the
behaviour of the iterator. So why didn't we this scenario? We did, but the
test case was lacking some capabilities and since it's a negative test case, it
happily accepted any kind of error from the library, therefore, negative
versions of DO_TEST_CAPS_LATEST have been introduced to mitigate failures like
these in the future.
Erik
8:47 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: domain: gfx: Fix shadowing the ptr argument to graphics validation
Since the code was never run, this stupid mistake could have only been
spotted by an accident.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/qemu/qemu_domain.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 509da6bfea..1eb0e31df0 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5783,9 +5783,7 @@ qemuDomainDeviceDefValidateGraphics(const virDomainGraphicsDef
*graphics,
size_t i;
for (i = 0; i < def->ngraphics; i++) {
- graphics = def->graphics[i];
-
- if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS) {
+ if (def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS) {
have_egl_headless = true;
break;
}
--
2.19.2
5:48 p.m.
New subject: [libvirt] [PATCH 1/4] qemu: domain: gfx: Fix shadowing the ptr argument to graphics validation
On 12/7/18 9:47 AM, Erik Skultety wrote:
Since the code was never run, this stupid mistake could have only
been
spotted by an accident.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/qemu/qemu_domain.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
Gah - that wasn't obvious... and it's scary the compiler was silent.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:06 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: domain: gfx: Fix shadowing the ptr argument to graphics validation
On Tue, Dec 11, 2018 at 06:48:26PM -0500, John Ferlan wrote:
On 12/7/18 9:47 AM, Erik Skultety wrote:
> Since the code was never run, this stupid mistake could have only been
> spotted by an accident.
>
> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
Gah - that wasn't obvious... and it's scary the compiler was silent.
Indeed it is. I'm just wondering when would I actually want to use this
legitimately since it's perfectly fine to assign into a "copy" pointer from
the standard's point of view.
Thanks,
Erik
8:47 a.m.
New subject: [libvirt] [PATCH 2/4] conf: domain: Introduce virDomainDeviceInfoIterate flags
One of the usages of the device iterator is to run config validation.
That's a problem for graphics devices, because they don't have any @info
data (graphics shouldn't have been considered as devices in the first
place), and simply passing NULL would crash a few callbacks invoked from
the iterator. Fix this problem by introducing iterator flags.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/conf/domain_conf.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b70dca6c61..11552bff5b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3703,10 +3703,18 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
}
+typedef enum {
+ DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
+} virDomainDeviceInfoIterateFlags;
+
+/*
+ * Iterates over domain devices which provide virDomainDeviceInfo data. The
+ * default behaviour can be altered with virDomainDeviceInfoIterateFlags.
+ */
static int
virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
virDomainDeviceInfoCallback cb,
- bool all,
+ unsigned int iteratorFlags,
void *opaque)
{
size_t i;
@@ -3772,6 +3780,8 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
return rc;
}
for (i = 0; i < def->nconsoles; i++) {
+ bool all = iteratorFlags & DEVICE_INFO_ITERATE_ALL_CONSOLES;
+
if (virDomainSkipBackcompatConsole(def, i, all))
continue;
device.data.chr = def->consoles[i];
@@ -3908,7 +3918,7 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
virDomainDeviceInfoCallback cb,
void *opaque)
{
- return virDomainDeviceInfoIterateInternal(def, cb, false, opaque);
+ return virDomainDeviceInfoIterateInternal(def, cb, 0, opaque);
}
@@ -3918,7 +3928,7 @@ virDomainDefHasDeviceAddress(virDomainDefPtr def,
{
if (virDomainDeviceInfoIterateInternal(def,
virDomainDefHasDeviceAddressIterator,
- true,
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
info) < 0)
return true;
@@ -5291,7 +5301,7 @@ virDomainDefPostParse(virDomainDefPtr def,
/* iterate the devices */
ret = virDomainDeviceInfoIterateInternal(def,
virDomainDefPostParseDeviceIterator,
- true,
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
&data);
if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0)
@@ -5927,7 +5937,8 @@ virDomainDefValidateAliases(const virDomainDef *def,
if (virDomainDeviceInfoIterateInternal((virDomainDefPtr) def,
virDomainDeviceDefValidateAliasesIterator,
- true, &data) < 0)
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
+ &data) < 0)
goto cleanup;
if (aliases) {
@@ -6337,7 +6348,8 @@ virDomainDefValidate(virDomainDefPtr def,
/* iterate the devices */
if (virDomainDeviceInfoIterateInternal(def,
virDomainDefValidateDeviceIterator,
- true, &data) < 0)
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
+ &data) < 0)
return -1;
if (virDomainDefValidateInternal(def) < 0)
@@ -29926,7 +29938,8 @@ virDomainDefFindDevice(virDomainDefPtr def,
dev->type = VIR_DOMAIN_DEVICE_NONE;
virDomainDeviceInfoIterateInternal(def, virDomainDefFindDeviceCallback,
- true, &data);
+ DEVICE_INFO_ITERATE_ALL_CONSOLES,
+ &data);
if (dev->type == VIR_DOMAIN_DEVICE_NONE) {
if (reportError) {
--
2.19.2
5:50 p.m.
New subject: [libvirt] [PATCH 2/4] conf: domain: Introduce virDomainDeviceInfoIterate flags
On 12/7/18 9:47 AM, Erik Skultety wrote:
One of the usages of the device iterator is to run config
validation.
That's a problem for graphics devices, because they don't have any @info
data (graphics shouldn't have been considered as devices in the first
place), and simply passing NULL would crash a few callbacks invoked from
the iterator. Fix this problem by introducing iterator flags.
Somewhat confusing commit message or I'm just being dense, your choice.
Although I think that @info data for graphics devices could be moved
into patch4 since that's where it makes more sense (eventually at least).
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/conf/domain_conf.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b70dca6c61..11552bff5b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3703,10 +3703,18 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
}
+typedef enum {
+ DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
+} virDomainDeviceInfoIterateFlags;
+
Logic seems right vis-a-vis replacing a bool with a flag properly. I
don't get the name and comment, but I'm not sure I could name it better.
The called function with the @all flag only processes when @all == false
as I'm reading things. Anyway, if someone has a better idea, then they
should speak up before you push!
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:29 a.m.
New subject: [libvirt] [PATCH 2/4] conf: domain: Introduce virDomainDeviceInfoIterate flags
On Tue, Dec 11, 2018 at 06:50:19PM -0500, John Ferlan wrote:
On 12/7/18 9:47 AM, Erik Skultety wrote:
> One of the usages of the device iterator is to run config validation.
> That's a problem for graphics devices, because they don't have any @info
> data (graphics shouldn't have been considered as devices in the first
> place), and simply passing NULL would crash a few callbacks invoked from
> the iterator. Fix this problem by introducing iterator flags.
Somewhat confusing commit message or I'm just being dense, your choice.
I see, especially the mention of graphics devices in a patch where there's
nothing related, how about this:
Validation of domain devices is accomplished via a generic device iterator
which takes a callback, iterates over all kinds of supported device types and
invokes the callback on every single device. However, there might be cases when
we need to alter the behaviour of the iteration (most notably skip or include a
group of devices). Therefore, this patch introduces iterator flags.
Although I think that @info data for graphics devices could be moved
into patch4 since that's where it makes more sense (eventually at least).
>
> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
> ---
> src/conf/domain_conf.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b70dca6c61..11552bff5b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3703,10 +3703,18 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
> }
>
>
> +typedef enum {
> + DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
> +} virDomainDeviceInfoIterateFlags;
> +
Logic seems right vis-a-vis replacing a bool with a flag properly. I
don't get the name and comment, but I'm not sure I could name it better.
The iterator function itself doesn't have a perfect name, I was just trying to
use that name to derive a new one to reflect the relation, but I can use
virDomainDeviceIteratorFlags instead. Going one step further we might even want
to rename the iterator and drop the "Info" part since that won't be 100%
once I
enable it for graphics.
The called function with the @all flag only processes when @all ==
false
So, because console[0] corresponds to the first serial console, we often
special-case these (in general). The @all flag here just says whether the
processing callback should skip console[0] or not, if @all == false, then
virDomainSkipBackcompatConsole will return true, thus skipping invocation of
the callback on this device.
as I'm reading things. Anyway, if someone has a better idea, then
they
should speak up before you push!
Let me know whether the suggested update to the commit message along with the
changes to naming are okay and I'll proceed with merging the patch.
Thanks,
Erik
6:19 a.m.
New subject: [libvirt] [PATCH 2/4] conf: domain: Introduce virDomainDeviceInfoIterate flags
On 12/12/18 3:29 AM, Erik Skultety wrote:
On Tue, Dec 11, 2018 at 06:50:19PM -0500, John Ferlan wrote:
>
>
> On 12/7/18 9:47 AM, Erik Skultety wrote:
>> One of the usages of the device iterator is to run config validation.
>> That's a problem for graphics devices, because they don't have any @info
>> data (graphics shouldn't have been considered as devices in the first
>> place), and simply passing NULL would crash a few callbacks invoked from
>> the iterator. Fix this problem by introducing iterator flags.
>
> Somewhat confusing commit message or I'm just being dense, your choice.
I see, especially the mention of graphics devices in a patch where there's
nothing related, how about this:
Validation of domain devices is accomplished via a generic device iterator
which takes a callback, iterates over all kinds of supported device types and
invokes the callback on every single device. However, there might be cases when
we need to alter the behaviour of the iteration (most notably skip or include a
group of devices). Therefore, this patch introduces iterator flags.
That's fine.
>
> Although I think that @info data for graphics devices could be moved
> into patch4 since that's where it makes more sense (eventually at least).
>
>>
>> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
>> ---
>> src/conf/domain_conf.c | 27 ++++++++++++++++++++-------
>> 1 file changed, 20 insertions(+), 7 deletions(-)
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b70dca6c61..11552bff5b 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -3703,10 +3703,18 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
>> }
>>
>>
>> +typedef enum {
>> + DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
>> +} virDomainDeviceInfoIterateFlags;
>> +
>
> Logic seems right vis-a-vis replacing a bool with a flag properly. I
> don't get the name and comment, but I'm not sure I could name it better.
The iterator function itself doesn't have a perfect name, I was just trying to
use that name to derive a new one to reflect the relation, but I can use
virDomainDeviceIteratorFlags instead. Going one step further we might even want
to rename the iterator and drop the "Info" part since that won't be 100%
once I
enable it for graphics.
<facepalm> I meant just the flag name itself - the typedef name is just
that and used only once, so no big deal.
> The called function with the @all flag only processes when @all
== false
So, because console[0] corresponds to the first serial console, we often
special-case these (in general). The @all flag here just says whether the
processing callback should skip console[0] or not, if @all == false, then
virDomainSkipBackcompatConsole will return true, thus skipping invocation of
the callback on this device.
Yeah I recall the [0] console entry being quite special with the details
being found in various places in the code.
In any case, if @all == false, then true is only returned if other
conditions are met too such as using/checking the [0]th entry of the
consoles list. When @all == true, then false is returned and that's the
"more normal" case as prior to this patch @all was true for all except
when virDomainDeviceInfoIterate calls virDomainDeviceInfoIterateInternal
> as I'm reading things. Anyway, if someone has a better idea, then they
> should speak up before you push!
Let me know whether the suggested update to the commit message along with the
changes to naming are okay and I'll proceed with merging the patch.
Sure things are fine
John
8:47 a.m.
New subject: [libvirt] [PATCH 3/4] tests: Introduce negative versions of DO_TEST_CAPS_LATEST
As commit d8266ebe161 demonstrated, it's so easy to forget to add a
single capability which in turn can easily fool the test suite so that
tests expecting a failure can fail with a different error than we
expected, but still making those pass. Unfortunately for commit
d8266ebe161, it allowed a domain with multiple OpenGL-enabled graphics
devices to start successfully. As a precaution measure, introduce
negative versions of DO_TEST_CAPS_LATEST macros, so that we eliminate
this vector from now on.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
tests/qemuxml2argvtest.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index e17709e7e1..528139654c 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -806,13 +806,22 @@ mymain(void)
# define DO_TEST_CAPS_VER(name, ver) \
DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver)
-# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
- DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, 0, 0, arch, \
+# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, flags, parseFlags) \
+ DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, flags, parseFlags, arch,
\
virHashLookup(capslatest, arch), true)
+# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, 0, 0) \
+
# define DO_TEST_CAPS_LATEST(name) \
DO_TEST_CAPS_ARCH_LATEST(name, "x86_64")
+# define DO_TEST_FAILURE_CAPS_LATEST(name) \
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_FAILURE, 0)
+
+# define DO_TEST_PARSE_ERROR_CAPS_LATEST(name) \
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_PARSE_ERROR, 0)
+
/**
* The following test macros should be used only in cases when the tests require
* testing of some non-standard combination of capability flags
--
2.19.2
5:51 p.m.
New subject: [libvirt] [PATCH 3/4] tests: Introduce negative versions of DO_TEST_CAPS_LATEST
On 12/7/18 9:47 AM, Erik Skultety wrote:
As commit d8266ebe161 demonstrated, it's so easy to forget to add
a
single capability which in turn can easily fool the test suite so that
tests expecting a failure can fail with a different error than we
expected, but still making those pass. Unfortunately for commit
d8266ebe161, it allowed a domain with multiple OpenGL-enabled graphics
devices to start successfully. As a precaution measure, introduce
precautionary
negative versions of DO_TEST_CAPS_LATEST macros, so that we
eliminate
this vector from now on.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
tests/qemuxml2argvtest.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
Pulling the needle out of the haystack of d8266ebe1 which of the 3 new
tests in qemuxml2argvtest was bad? None used the TEST_CAPS_LATEST
nomenclature and you didn't change the test in this patch, so I'm left
wondering. Of course, patch4 has the answer.
Nothing major here, but understand coming in cold on this and reading
the commit message is, well, confusing.
I think it's simple enough to indicate you are introducing the two new
macros to allow for test and parse failure checking. The "details" about
the previous commit could be moved into patch 4 I suppose to show what
you're fixing since you're not changing a test here.
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index e17709e7e1..528139654c 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -806,13 +806,22 @@ mymain(void)
# define DO_TEST_CAPS_VER(name, ver) \
DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver)
-# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
- DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, 0, 0, arch, \
+# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, flags, parseFlags) \
+ DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, flags, parseFlags, arch,
\
virHashLookup(capslatest, arch), true)
+# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, 0, 0) \
There should not be a \ at the end of the previous line. Perhaps a
holdover from the copy-pasta with the 'virHashLookup...' line.
+
# define DO_TEST_CAPS_LATEST(name) \
DO_TEST_CAPS_ARCH_LATEST(name, "x86_64")
+# define DO_TEST_FAILURE_CAPS_LATEST(name) \
To follow DO_TEST_CAPS_LATEST how about
DO_TEST_CAPS_LATEST_FAILURE
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64",
FLAG_EXPECT_FAILURE, 0)
But this one never gets used - so do we really need it? IDC, but it
could be removed...
+
+# define DO_TEST_PARSE_ERROR_CAPS_LATEST(name) \
and likewise
DO_TEST_CAPS_LATEST_PARSE_FAILURE
with the changes,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
+ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64",
FLAG_EXPECT_PARSE_ERROR, 0)
+
/**
* The following test macros should be used only in cases when the tests require
* testing of some non-standard combination of capability flags
2:37 a.m.
New subject: [libvirt] [PATCH 3/4] tests: Introduce negative versions of DO_TEST_CAPS_LATEST
On Tue, Dec 11, 2018 at 06:51:56PM -0500, John Ferlan wrote:
On 12/7/18 9:47 AM, Erik Skultety wrote:
> As commit d8266ebe161 demonstrated, it's so easy to forget to add a
> single capability which in turn can easily fool the test suite so that
> tests expecting a failure can fail with a different error than we
> expected, but still making those pass. Unfortunately for commit
> d8266ebe161, it allowed a domain with multiple OpenGL-enabled graphics
> devices to start successfully. As a precaution measure, introduce
precautionary
> negative versions of DO_TEST_CAPS_LATEST macros, so that we eliminate
> this vector from now on.
>
> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
> ---
> tests/qemuxml2argvtest.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
Pulling the needle out of the haystack of d8266ebe1 which of the 3 new
tests in qemuxml2argvtest was bad? None used the TEST_CAPS_LATEST
nomenclature and you didn't change the test in this patch, so I'm left
wondering. Of course, patch4 has the answer.
Nothing major here, but understand coming in cold on this and reading
the commit message is, well, confusing.
I think it's simple enough to indicate you are introducing the two new
macros to allow for test and parse failure checking. The "details" about
the previous commit could be moved into patch 4 I suppose to show what
you're fixing since you're not changing a test here.
Noted, will do.
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index e17709e7e1..528139654c 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -806,13 +806,22 @@ mymain(void)
> # define DO_TEST_CAPS_VER(name, ver) \
> DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver)
>
> -# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
> - DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, 0, 0, arch, \
> +# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, flags, parseFlags) \
> + DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, flags, parseFlags,
arch, \
> virHashLookup(capslatest, arch), true)
>
> +# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \
> + DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, 0, 0) \
There should not be a \ at the end of the previous line. Perhaps a
holdover from the copy-pasta with the 'virHashLookup...' line.
Yep, nice catch :).
> +
> # define DO_TEST_CAPS_LATEST(name) \
> DO_TEST_CAPS_ARCH_LATEST(name, "x86_64")
>
> +# define DO_TEST_FAILURE_CAPS_LATEST(name) \
To follow DO_TEST_CAPS_LATEST how about
DO_TEST_CAPS_LATEST_FAILURE
Better.
> + DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_FAILURE,
0)
But this one never gets used - so do we really need it? IDC, but it
could be removed...
FAILURE was the first one I created (only then I figured the error I'm getting
is coming from the parser :/) so I thought why not add both since I'm already
creating new macros, so I'd like to keep it in regardless, might get handy
soon.
> +
> +# define DO_TEST_PARSE_ERROR_CAPS_LATEST(name) \
and likewise
DO_TEST_CAPS_LATEST_PARSE_FAILURE
with the changes,
Thanks,
Erik
8:47 a.m.
New subject: [libvirt] [PATCH 4/4] conf: domain: gfx: Iterate over graphics devices when doing validation
The validation code for graphics has been in place for a while, but
because it is only executed from the device iterator, that validation
code was never truly run. The unfortunate side effect of this whole mess
was that a few capabilities were missing from the test suite, which in
turn meant that a few graphics test which expected a failure happily
accepted whatever failure the parser returned which made them succeed
even though in reality we allowed to start a domain with multiple
OpenGL-enabled graphics devices.
This patch enables iteration over graphics devices. Unsurprisingly,
a few tests started failing as a result, so fix those too.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/conf/domain_conf.c | 13 ++++++++++++-
tests/qemuxml2argvtest.c | 7 ++-----
tests/qemuxml2xmltest.c | 10 +++++++---
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 11552bff5b..a4c762a210 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3705,6 +3705,7 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
typedef enum {
DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
+ DEVICE_INFO_ITERATE_GRAPHICS = 1 << 1 /* Iterate graphics */
} virDomainDeviceInfoIterateFlags;
/*
@@ -3870,6 +3871,15 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
return rc;
}
+ if (iteratorFlags & DEVICE_INFO_ITERATE_GRAPHICS) {
+ device.type = VIR_DOMAIN_DEVICE_GRAPHICS;
+ for (i = 0; i < def->ngraphics; i++) {
+ device.data.graphics = def->graphics[i];
+ if ((rc = cb(def, &device, NULL, opaque)) != 0)
+ return rc;
+ }
+ }
+
/* Coverity is not very happy with this - all dead_error_condition */
#if !STATIC_ANALYSIS
/* This switch statement is here to trigger compiler warning when adding
@@ -6348,7 +6358,8 @@ virDomainDefValidate(virDomainDefPtr def,
/* iterate the devices */
if (virDomainDeviceInfoIterateInternal(def,
virDomainDefValidateDeviceIterator,
- DEVICE_INFO_ITERATE_ALL_CONSOLES,
+ (DEVICE_INFO_ITERATE_ALL_CONSOLES |
+ DEVICE_INFO_ITERATE_GRAPHICS),
&data) < 0)
return -1;
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 528139654c..05451863ad 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1299,7 +1299,7 @@ mymain(void)
DO_TEST("graphics-sdl",
QEMU_CAPS_DEVICE_VGA);
- DO_TEST_FAILURE("graphics-sdl-egl-headless", NONE);
+ DO_TEST_PARSE_ERROR_CAPS_LATEST("graphics-sdl-egl-headless");
DO_TEST("graphics-sdl-fullscreen",
QEMU_CAPS_DEVICE_CIRRUS_VGA);
DO_TEST("graphics-spice",
@@ -1358,10 +1358,7 @@ mymain(void)
QEMU_CAPS_SPICE,
QEMU_CAPS_EGL_HEADLESS,
QEMU_CAPS_DEVICE_QXL);
- DO_TEST_FAILURE("graphics-spice-invalid-egl-headless",
- QEMU_CAPS_SPICE,
- QEMU_CAPS_EGL_HEADLESS,
- QEMU_CAPS_DEVICE_QXL);
+ DO_TEST_PARSE_ERROR_CAPS_LATEST("graphics-spice-invalid-egl-headless");
DO_TEST_CAPS_LATEST("graphics-spice-gl-auto-rendernode");
DO_TEST("input-usbmouse", NONE);
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index c98b9571ef..1062deee37 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -402,7 +402,8 @@ mymain(void)
cfg->vncAutoUnixSocket = false;
DO_TEST("graphics-vnc-socket", NONE);
DO_TEST("graphics-vnc-auto-socket", NONE);
- DO_TEST("graphics-vnc-egl-headless", NONE);
+ DO_TEST("graphics-vnc-egl-headless",
+ QEMU_CAPS_EGL_HEADLESS);
DO_TEST("graphics-sdl", NONE);
DO_TEST("graphics-sdl-fullscreen", NONE);
@@ -414,9 +415,12 @@ mymain(void)
cfg->spiceAutoUnixSocket = true;
DO_TEST("graphics-spice-auto-socket-cfg", NONE);
cfg->spiceAutoUnixSocket = false;
- DO_TEST("graphics-spice-egl-headless", NONE);
+ DO_TEST("graphics-spice-egl-headless",
+ QEMU_CAPS_EGL_HEADLESS);
- DO_TEST("graphics-egl-headless-rendernode", NONE);
+ DO_TEST("graphics-egl-headless-rendernode",
+ QEMU_CAPS_EGL_HEADLESS,
+ QEMU_CAPS_EGL_HEADLESS_RENDERNODE);
DO_TEST("input-usbmouse", NONE);
DO_TEST("input-usbtablet", NONE);
--
2.19.2
5:54 p.m.
New subject: [libvirt] [PATCH 4/4] conf: domain: gfx: Iterate over graphics devices when doing validation
On 12/7/18 9:47 AM, Erik Skultety wrote:
The validation code for graphics has been in place for a while, but
because it is only executed from the device iterator, that validation
code was never truly run. The unfortunate side effect of this whole mess
dang confusing postparse and validation processing... when you say
"device iterator" you meant the hypervisor specific device iterator,
right? That is, what's called by deviceValidateCallback or the call
into qemuDomainDeviceDefValidateGraphics. I'm just trying to follow the
wording, nothing more, nothing less.
was that a few capabilities were missing from the test suite, which
in
turn meant that a few graphics test which expected a failure happily
graphics tests (the s on test)
accepted whatever failure the parser returned which made them
succeed
even though in reality we allowed to start a domain with multiple
OpenGL-enabled graphics devices.
add blank line between paragraphs
This patch enables iteration over graphics devices. Unsurprisingly,
a few tests started failing as a result, so fix those too.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/conf/domain_conf.c | 13 ++++++++++++-
tests/qemuxml2argvtest.c | 7 ++-----
tests/qemuxml2xmltest.c | 10 +++++++---
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 11552bff5b..a4c762a210 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3705,6 +3705,7 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
typedef enum {
DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
+ DEVICE_INFO_ITERATE_GRAPHICS = 1 << 1 /* Iterate graphics */
} virDomainDeviceInfoIterateFlags;
/*
@@ -3870,6 +3871,15 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
return rc;
}
+ if (iteratorFlags & DEVICE_INFO_ITERATE_GRAPHICS) {
+ device.type = VIR_DOMAIN_DEVICE_GRAPHICS;
+ for (i = 0; i < def->ngraphics; i++) {
+ device.data.graphics = def->graphics[i];
+ if ((rc = cb(def, &device, NULL, opaque)) != 0)
So the only caller to this passes cb=virDomainDefValidateDeviceIterator
which ironically doesn't use @info (e.g. the 3rd param):
virDomainDefValidateDeviceIterator(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
virDomainDeviceInfoPtr info
ATTRIBUTE_UNUSED,
void *opaque)
I know these are generic functions, but some day if someone changes that
helper to "use" @info can I assume that some test would fail in a most
spectacular way?
Not a problem here, just looking to confirm my own feeling on this with
what you'd expect. You may want to even add a comment noting that for a
graphics device the @info isn't used (remember my patch2 comment), so
one has to be careful which callers can set the iterate flag that dumps
you here. Say nothing for the called function - if it expects something
and gets NULL, all bets are off. I can assume it wouldn't be picked up
by the compiler's ATTRIBUTE_NONNULL too unless the prototype for the @cb
was set to have that.
+ return rc;
+ }
+ }
+
/* Coverity is not very happy with this - all dead_error_condition */
#if !STATIC_ANALYSIS
/* This switch statement is here to trigger compiler warning when adding
@@ -6348,7 +6358,8 @@ virDomainDefValidate(virDomainDefPtr def,
/* iterate the devices */
if (virDomainDeviceInfoIterateInternal(def,
virDomainDefValidateDeviceIterator,
- DEVICE_INFO_ITERATE_ALL_CONSOLES,
+ (DEVICE_INFO_ITERATE_ALL_CONSOLES |
+ DEVICE_INFO_ITERATE_GRAPHICS),
&data) < 0)
return -1;
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 528139654c..05451863ad 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1299,7 +1299,7 @@ mymain(void)
DO_TEST("graphics-sdl",
QEMU_CAPS_DEVICE_VGA);
- DO_TEST_FAILURE("graphics-sdl-egl-headless", NONE);
+ DO_TEST_PARSE_ERROR_CAPS_LATEST("graphics-sdl-egl-headless");
We're changing from a general execution failure to a parse failure, so
theoretically we could not have started a guest like this, right? If we
could have then I'd be concerned with the guest disappearance factor on
libvirtd restart, but I don't think that's the case here. Still I go
back to my comment at the top dang confusing postparse and validation
processing, so I just want to make sure...
If my assumption is right about not being able to start like this, then
with a couple of small comment fixups in then you have my
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
otherwise, some more thought may be necessary.
John
DO_TEST("graphics-sdl-fullscreen",
QEMU_CAPS_DEVICE_CIRRUS_VGA);
DO_TEST("graphics-spice",
@@ -1358,10 +1358,7 @@ mymain(void)
QEMU_CAPS_SPICE,
QEMU_CAPS_EGL_HEADLESS,
QEMU_CAPS_DEVICE_QXL);
- DO_TEST_FAILURE("graphics-spice-invalid-egl-headless",
- QEMU_CAPS_SPICE,
- QEMU_CAPS_EGL_HEADLESS,
- QEMU_CAPS_DEVICE_QXL);
+ DO_TEST_PARSE_ERROR_CAPS_LATEST("graphics-spice-invalid-egl-headless");
DO_TEST_CAPS_LATEST("graphics-spice-gl-auto-rendernode");
DO_TEST("input-usbmouse", NONE);
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index c98b9571ef..1062deee37 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -402,7 +402,8 @@ mymain(void)
cfg->vncAutoUnixSocket = false;
DO_TEST("graphics-vnc-socket", NONE);
DO_TEST("graphics-vnc-auto-socket", NONE);
- DO_TEST("graphics-vnc-egl-headless", NONE);
+ DO_TEST("graphics-vnc-egl-headless",
+ QEMU_CAPS_EGL_HEADLESS);
DO_TEST("graphics-sdl", NONE);
DO_TEST("graphics-sdl-fullscreen", NONE);
@@ -414,9 +415,12 @@ mymain(void)
cfg->spiceAutoUnixSocket = true;
DO_TEST("graphics-spice-auto-socket-cfg", NONE);
cfg->spiceAutoUnixSocket = false;
- DO_TEST("graphics-spice-egl-headless", NONE);
+ DO_TEST("graphics-spice-egl-headless",
+ QEMU_CAPS_EGL_HEADLESS);
- DO_TEST("graphics-egl-headless-rendernode", NONE);
+ DO_TEST("graphics-egl-headless-rendernode",
+ QEMU_CAPS_EGL_HEADLESS,
+ QEMU_CAPS_EGL_HEADLESS_RENDERNODE);
DO_TEST("input-usbmouse", NONE);
DO_TEST("input-usbtablet", NONE);
5:37 a.m.
New subject: [libvirt] [PATCH 4/4] conf: domain: gfx: Iterate over graphics devices when doing validation
On Tue, Dec 11, 2018 at 06:54:31PM -0500, John Ferlan wrote:
On 12/7/18 9:47 AM, Erik Skultety wrote:
> The validation code for graphics has been in place for a while, but
> because it is only executed from the device iterator, that validation
> code was never truly run. The unfortunate side effect of this whole mess
dang confusing postparse and validation processing... when you say
"device iterator" you meant the hypervisor specific device iterator,
right? That is, what's called by deviceValidateCallback or the call
into qemuDomainDeviceDefValidateGraphics. I'm just trying to follow the
wording, nothing more, nothing less.
By iterator I mean virDomainDeviceInfoIterateInternal.
> was that a few capabilities were missing from the test suite, which in
> turn meant that a few graphics test which expected a failure happily
graphics tests (the s on test)
> accepted whatever failure the parser returned which made them succeed
> even though in reality we allowed to start a domain with multiple
> OpenGL-enabled graphics devices.
add blank line between paragraphs
> This patch enables iteration over graphics devices. Unsurprisingly,
> a few tests started failing as a result, so fix those too.
>
> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
> ---
> src/conf/domain_conf.c | 13 ++++++++++++-
> tests/qemuxml2argvtest.c | 7 ++-----
> tests/qemuxml2xmltest.c | 10 +++++++---
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 11552bff5b..a4c762a210 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3705,6 +3705,7 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
>
> typedef enum {
> DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
> + DEVICE_INFO_ITERATE_GRAPHICS = 1 << 1 /* Iterate graphics */
> } virDomainDeviceInfoIterateFlags;
>
> /*
> @@ -3870,6 +3871,15 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
> return rc;
> }
>
> + if (iteratorFlags & DEVICE_INFO_ITERATE_GRAPHICS) {
> + device.type = VIR_DOMAIN_DEVICE_GRAPHICS;
> + for (i = 0; i < def->ngraphics; i++) {
> + device.data.graphics = def->graphics[i];
> + if ((rc = cb(def, &device, NULL, opaque)) != 0)
So the only caller to this passes cb=virDomainDefValidateDeviceIterator
which ironically doesn't use @info (e.g. the 3rd param):
virDomainDefValidateDeviceIterator(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
virDomainDeviceInfoPtr info
ATTRIBUTE_UNUSED,
void *opaque)
Looking at the function signature, it might be worth doing a bit of a refactor
on the iterator, since @info is treated as unused in a few cases, I'll try
looking into that as a follow-up.
I know these are generic functions, but some day if someone changes that
helper to "use" @info can I assume that some test would fail in a most
spectacular way?
It most certainly will.
Not a problem here, just looking to confirm my own feeling on this with
what you'd expect. You may want to even add a comment noting that for a
graphics device the @info isn't used (remember my patch2 comment), so
one has to be careful which callers can set the iterate flag that dumps
you here. Say nothing for the called function - if it expects something
and gets NULL, all bets are off. I can assume it wouldn't be picked up
by the compiler's ATTRIBUTE_NONNULL too unless the prototype for the @cb
was set to have that.
> + return rc;
> + }
> + }
> +
> /* Coverity is not very happy with this - all dead_error_condition */
> #if !STATIC_ANALYSIS
> /* This switch statement is here to trigger compiler warning when adding
> @@ -6348,7 +6358,8 @@ virDomainDefValidate(virDomainDefPtr def,
> /* iterate the devices */
> if (virDomainDeviceInfoIterateInternal(def,
> virDomainDefValidateDeviceIterator,
> - DEVICE_INFO_ITERATE_ALL_CONSOLES,
> + (DEVICE_INFO_ITERATE_ALL_CONSOLES |
> + DEVICE_INFO_ITERATE_GRAPHICS),
> &data) < 0)
> return -1;
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 528139654c..05451863ad 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1299,7 +1299,7 @@ mymain(void)
>
> DO_TEST("graphics-sdl",
> QEMU_CAPS_DEVICE_VGA);
> - DO_TEST_FAILURE("graphics-sdl-egl-headless", NONE);
> + DO_TEST_PARSE_ERROR_CAPS_LATEST("graphics-sdl-egl-headless");
We're changing from a general execution failure to a parse failure, so
theoretically we could not have started a guest like this, right? If we
could have then I'd be concerned with the guest disappearance factor on
libvirtd restart, but I don't think that's the case here. Still I go
It is not. Note the flag VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE which we pass
during driver initialization, thus the validation is only run on new
definitions and before starting a guest.
back to my comment at the top dang confusing postparse and
validation
processing, so I just want to make sure...
It was a bit confusing to me too, the test case never worked as expected even
though it failed. Guests having this issue would not disappear, because it's
within the validation code. However, validation is still part of the parser
code, so as far as the test suite is concerned, it's a PARSE_ERROR, so I had to
change it accordingly, otherwise the test started to FAIL (because we expected
a failure after the parsing phase).
If my assumption is right about not being able to start like this, then
We shouldn't start a guest like that, but we did anyway, not to mention QEMU
didn't really care, but it's wrong anyway.
with a couple of small comment fixups in then you have my
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks,
Erik
8:39 a.m.
New subject: [libvirt] [PATCH 4/4] conf: domain: gfx: Iterate over graphics devices when doing validation
On Tue, Dec 11, 2018 at 06:54:31PM -0500, John Ferlan wrote:
On 12/7/18 9:47 AM, Erik Skultety wrote:
> The validation code for graphics has been in place for a while, but
> because it is only executed from the device iterator, that validation
> code was never truly run. The unfortunate side effect of this whole mess
dang confusing postparse and validation processing... when you say
"device iterator" you meant the hypervisor specific device iterator,
right? That is, what's called by deviceValidateCallback or the call
into qemuDomainDeviceDefValidateGraphics. I'm just trying to follow the
wording, nothing more, nothing less.
> was that a few capabilities were missing from the test suite, which in
> turn meant that a few graphics test which expected a failure happily
graphics tests (the s on test)
> accepted whatever failure the parser returned which made them succeed
> even though in reality we allowed to start a domain with multiple
> OpenGL-enabled graphics devices.
add blank line between paragraphs
> This patch enables iteration over graphics devices. Unsurprisingly,
> a few tests started failing as a result, so fix those too.
>
> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
> ---
> src/conf/domain_conf.c | 13 ++++++++++++-
> tests/qemuxml2argvtest.c | 7 ++-----
> tests/qemuxml2xmltest.c | 10 +++++++---
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 11552bff5b..a4c762a210 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3705,6 +3705,7 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
>
> typedef enum {
> DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
> + DEVICE_INFO_ITERATE_GRAPHICS = 1 << 1 /* Iterate graphics */
> } virDomainDeviceInfoIterateFlags;
>
> /*
> @@ -3870,6 +3871,15 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
> return rc;
> }
>
> + if (iteratorFlags & DEVICE_INFO_ITERATE_GRAPHICS) {
> + device.type = VIR_DOMAIN_DEVICE_GRAPHICS;
> + for (i = 0; i < def->ngraphics; i++) {
> + device.data.graphics = def->graphics[i];
> + if ((rc = cb(def, &device, NULL, opaque)) != 0)
So the only caller to this passes cb=virDomainDefValidateDeviceIterator
which ironically doesn't use @info (e.g. the 3rd param):
virDomainDefValidateDeviceIterator(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
virDomainDeviceInfoPtr info
ATTRIBUTE_UNUSED,
void *opaque)
I know these are generic functions, but some day if someone changes that
helper to "use" @info can I assume that some test would fail in a most
spectacular way?
Not a problem here, just looking to confirm my own feeling on this with
what you'd expect. You may want to even add a comment noting that for a
graphics device the @info isn't used (remember my patch2 comment), so
one has to be careful which callers can set the iterate flag that dumps
you here. Say nothing for the called function - if it expects something
and gets NULL, all bets are off. I can assume it wouldn't be picked up
by the compiler's ATTRIBUTE_NONNULL too unless the prototype for the @cb
was set to have that.
> + return rc;
> + }
> + }
> +
> /* Coverity is not very happy with this - all dead_error_condition */
> #if !STATIC_ANALYSIS
> /* This switch statement is here to trigger compiler warning when adding
> @@ -6348,7 +6358,8 @@ virDomainDefValidate(virDomainDefPtr def,
> /* iterate the devices */
> if (virDomainDeviceInfoIterateInternal(def,
> virDomainDefValidateDeviceIterator,
> - DEVICE_INFO_ITERATE_ALL_CONSOLES,
> + (DEVICE_INFO_ITERATE_ALL_CONSOLES |
> + DEVICE_INFO_ITERATE_GRAPHICS),
> &data) < 0)
> return -1;
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 528139654c..05451863ad 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1299,7 +1299,7 @@ mymain(void)
>
> DO_TEST("graphics-sdl",
> QEMU_CAPS_DEVICE_VGA);
> - DO_TEST_FAILURE("graphics-sdl-egl-headless", NONE);
> + DO_TEST_PARSE_ERROR_CAPS_LATEST("graphics-sdl-egl-headless");
We're changing from a general execution failure to a parse failure, so
theoretically we could not have started a guest like this, right? If we
could have then I'd be concerned with the guest disappearance factor on
libvirtd restart, but I don't think that's the case here. Still I go
back to my comment at the top dang confusing postparse and validation
processing, so I just want to make sure...
If my assumption is right about not being able to start like this, then
with a couple of small comment fixups in then you have my
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
So, I made the adjustments as suggested, added a few comments, used a slightly
different name for the iterator flags, dropped the 'typedef' on the iterator
flag enum, ran travis on the series and pushed, thanks.
Erik
2172
days inactive
2177
days old
14 comments
2 participants
participants (2)
-
Erik Skultety -
John Ferlan