On Wed, Jul 12, 2017 at 15:14:20 +0200, Pavel Hrdina wrote:
> On Wed, Jul 12, 2017 at 02:56:48PM +0200, Jiri Denemark wrote:
> > Separated from qemuProcessUpdateLiveGuestCPU. The function makes sure
> > a guest CPU provides all features required by a domain definition.
> >
> > Signed-off-by: Jiri Denemark
> > ---
> > src/qemu/qemu_process.c | 35 ---
> > 1 file changed, 28 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index b2d27b6be..198f68d93 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -3986,6 +3986,31 @@ qemuProcessFetchGuestCPU(virQEMUDriverPtr driver,
> >
> >
> > static int
> > +qemuProcessVerifyCPU(virDomainObjPtr vm,
> > + virCPUDataPtr cpu)
> > +{
> > +virDomainDefPtr def = vm->def;
> > +
> > +if (!cpu)
> > +return 0;
> > +
> > +if (qemuProcessVerifyKVMFeatures(def, cpu) < 0 ||
> > +qemuProcessVerifyHypervFeatures(def, cpu) < 0)
> > +return -1;
> > +
> > +if (!def->cpu ||
> > +(def->cpu->mode == VIR_CPU_MODE_CUSTOM &&
> > + !def->cpu->model))
>
> This condition is now on two places, it would be worth to create a
> macro/function with some description why this specific condition.
Oh yeah, good idea. This condition is very likely repeated in a lot of
places which need to check whether the CPU def contains only topology
data. I'll try to come up with a good name for the function and make a
patch replacing all open coded conditions with it.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 12, 2017 at 16:42:36 +0200, Pavel Hrdina wrote:
> On Wed, Jul 12, 2017 at 02:56:53PM +0200, Jiri Denemark wrote:
> > When libvirt starts a new QEMU domain, it replaces host-model CPUs with
> > the appropriate custom CPU definition. However, when reconnecting to a
> > domain started by older libvirt (< 2.3), the domain would still have a
> > host-model CPU in its active definition.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1463957
> >
> > Signed-off-by: Jiri Denemark
> > ---
> > src/qemu/qemu_process.c | 48
> >
> > 1 file changed, 48 insertions(+)
>
> Reviewed-by: Pavel Hrdina
Thanks for the review. Pushed.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 12, 2017 at 05:07:56PM +0100, Daniel P. Berrange wrote:
On Wed, Jul 12, 2017 at 01:56:47PM +0200, Martin Kletzander wrote:
Also, having the weak alias we can drop all the mocks and the problems
with them and just redefine the functions we would like to mock in the
tests (see tests/virfilewrapper.c), it would work on win32, it would
not compile if we would forgot to make the function as an alias (so no
need to check that in the syntax-check) and maybe provide other benefits
that I can't think of right now.
Empirically I'm not seeing any compiler warnings if i take this approach
and don't mark the function as an alias.
Oooh, me too :( That's a pity; yet another thing that proves I still
don't _fully_ understand the details behind everything.
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/12/2017 12:14 PM, Daniel P. Berrange wrote:
> This series reverts the use of the __weak__ attribute which we did
> to fix the test suite with CLang. It cause unintended problems
> elsewhere which are not practical to fix. See the second patch
> for details.
>
> Daniel P. Berrange (2):
> Revert "internal: don't use weak symbols for Win32 platform"
> Revert "Prevent more compiler optimization of mockable functions"
>
So I thought about this too and I guess my question is - why are we not
dynamically linking with the mock libraries? What we do is use
LD_PRELOAD at runtime. However, if say virpcitest would link with
virpcimock at compile time (dynamically), we can throw away LD_PRELOAD
handling code.
On the other hand, this look trivial enough so I bet others have thought
of it. And since we don't use it there has to be catch that I'm missing. Or?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
QEMU fails to launch a sPAPR guest with clock sources other that RTC.
Internally qemu only uses RTC timer for hwclock. This patch reports
the right error message instead of qemu erroring out when any other
timer other than RTC is used.
Signed-off-by: Kothapally Madhu Pavan
---
src/qemu/qemu_command.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c53ab97..31561ce 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6440,6 +6440,15 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
break;
case VIR_DOMAIN_TIMER_NAME_PIT:
+/* Only RTC timer is supported as hwclock for sPAPR machines */
+if (ARCH_IS_PPC64(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported clock timer '%s' for '%s'
architecture"),
+
virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+ virArchToString(def->os.arch));
+return -1;
+}
+
switch (def->clock.timers[i]->tickpolicy) {
case -1:
case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY:
@@ -6483,13 +6492,21 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
break;
case VIR_DOMAIN_TIMER_NAME_HPET:
+/* Only RTC timer is supported as hwclock for sPAPR machines */
+if (ARCH_IS_PPC64(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported clock timer '%s' for '%s'
architecture"),
+
virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+ virArchToString(def->os.arch));
+return -1;
+}
+
/* the only meaningful attribute for hpet is "present". If
* present is -1, that means it wasn't specified, and
* should be left at the default for the
* hypervisor. "default" when -no-hpet exists is "yes",
* and when -no-hpet doesn't exist is "no". "confusing"?
* "yes"! */
-
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_HPET)) {
if (def->clock.timers[i]->present == 0)
virCommandAddArg(cmd, "-no-hpet");
@@ -7047,6 +7064,15 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
for (i = 0; i < def->clock.ntimers; i++) {
virDomainTimerDefPtr timer = def->clock.timers[i];
+/* Only RTC timer is supported as hwclock for sPAPR machines */
+if (ARCH_IS_PPC64(def->os.arch) && timer->name !=
VIR_DOMAIN_TIMER_NAME_RTC) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported clock timer '%s' for '%s'
architecture"),
+
virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+ virArchToString(def->os.arch));
+return -1;
+}
+
if (timer->name == VIR_DOMAIN_TIMER_NAME_KVMCLOCK &&
timer->present != -1) {
virBufferAsprintf(&buf, "%s,%ckvmclock",
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 13, 2017 at 10:13:25AM +0200, Michal Privoznik wrote:
> On 07/12/2017 12:14 PM, Daniel P. Berrange wrote:
> > This series reverts the use of the __weak__ attribute which we did
> > to fix the test suite with CLang. It cause unintended problems
> > elsewhere which are not practical to fix. See the second patch
> > for details.
> >
> > Daniel P. Berrange (2):
> > Revert "internal: don't use weak symbols for Win32 platform"
> > Revert "Prevent more compiler optimization of mockable functions"
> >
>
> So I thought about this too and I guess my question is - why are we not
> dynamically linking with the mock libraries? What we do is use
> LD_PRELOAD at runtime. However, if say virpcitest would link with
> virpcimock at compile time (dynamically), we can throw away LD_PRELOAD
> handling code.
Basically yes, however, that's tangential to this bug & patch.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 12, 2017 at 05:06:22PM +0100, Daniel P. Berrange wrote:
Currently any mockable functions are marked with attributes
noinline, noclone and weak. This prevents the compiler from
optimizing away the impl of these functions.
It has an unfortunate side effect with the libtool convenience
libraries, if executables directly link to them. For example
virlockd, virlogd both link to libvirt_util.la When this is
done, the linker does not consider weak symbols as being
undefined, so it never copies them into the final executable.
In this new approach, we stop annotating the headers entirely.
Instead we create a weak function alias in the source.
int fooImpl(void) {
..the real code..
}
int foo(void) __attribute__((noinline, noclone, weak, alias("fooImpl"))
If any functions in the same file call "foo", this prevents the
optimizer from inlining any part of fooImpl. When linking to the
libtool convenience static library, we also get all the symbols
present. Finally the test suite can just directly define a
'foo' function in its source, removing the need to use LD_PRELOAD
(though removal of LD_PRELOADS is left for a future patch).
What are you using for tag navigation? With this macro-defined
functions I cannot easily jump to them (the main reason why I don't like
macros defining functions). I would very much prefer if
ATTRIBUTE_MOCKABLE just took a parameter like this:
#define ATTRIBUTE_MOCKABLE(func) __attribute__((noinline, noclone, weak, alias(#func
"Impl"))
(written by hand, don't take mu word for it working out of the box) and
the original function would be left untouched.
Also, this fails on OS X [1] and I don't really see why:
util/vircommand.c:988:1: error: only weak aliases are supported on darwin
VIR_MOCKABLE(void,
^
./internal.h:252:60: note: expanded from macro 'VIR_MOCKABLE'
ret name(__VA_ARGS__) __attribute__((noinline, weak, __alias__(#name
"Impl"))); \
^
1 error generated.
make[3]: *** [util/libvirt_util_la-vircommand.lo] Error 1
make[3]: *** Waiting for unfinished jobs
[1] https://travis-ci.org/nertpinx/libvirt/jobs/253136004
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 13, 2017 at 01:14:12PM +0200, Martin Kletzander wrote:
> On Wed, Jul 12, 2017 at 05:06:22PM +0100, Daniel P. Berrange wrote:
> > Currently any mockable functions are marked with attributes
> > noinline, noclone and weak. This prevents the compiler from
> > optimizing away the impl of these functions.
> >
> > It has an unfortunate side effect with the libtool convenience
> > libraries, if executables directly link to them. For example
> > virlockd, virlogd both link to libvirt_util.la When this is
> > done, the linker does not consider weak symbols as being
> > undefined, so it never copies them into the final executable.
> >
> > In this new approach, we stop annotating the headers entirely.
> > Instead we create a weak function alias in the source.
> >
> > int fooImpl(void) {
> > ..the real code..
> > }
> >
> > int foo(void) __attribute__((noinline, noclone, weak, alias("fooImpl"))
> >
> > If any functions in the same file call "foo", this prevents the
> > optimizer from inlining any part of fooImpl. When linking to the
> > libtool convenience static library, we also get all the symbols
> > present. Finally the test suite can just directly define a
> > 'foo' function in its source, removing the need to use LD_PRELOAD
> > (though removal of LD_PRELOADS is left for a future patch).
> >
>
> What are you using for tag navigation? With this macro-defined
> functions I cannot easily jump to them (the main reason why I don't like
> macros defining functions).
Grep & historic knowledge :-)
> I would very much prefer if
> ATTRIBUTE_MOCKABLE just took a parameter like this:
>
> #define ATTRIBUTE_MOCKABLE(func) __attribute__((noinline, noclone, weak,
> alias(#func "Impl"))
>
> (written by hand, don't take mu word for it working out of the box) and
> the original function would be left untouched.
Yeah, that is certainly a valid alternative. I was not entirely happy
with the results I get here. As long as we don't mind repeating the
arg list in both places, your approach is more attractive, despite
the duplication.
> Also, this fails on OS X [1] and I don't really see why:
>
> util/vircommand.c:988:1: error: only weak aliases are supported on darwin
> VIR_MOCKABLE(void,
> ^
> ./internal.h:252:60: note: expanded from macro 'VIR_MOCKABLE'
> ret name(__VA_ARGS__) __attribute__((noinline, weak, __alias__(#name
> "Impl"))); \
It appears clang simply doesn't support 'alias' on Darwin.
Docs suggest that we need to use 'weakref(#name "Impl")' so I'll have
to test whether that's viable for our usage scenario or not.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 12, 2017 at 01:49:04PM +0200, Martin Kletzander wrote:
> On Wed, Jul 12, 2017 at 01:10:08PM +0200, Martin Kletzander wrote:
> > On Wed, Jul 12, 2017 at 11:14:16AM +0100, Daniel P. Berrange wrote:
> > > This reverts commit e4b980c853d2114b25fa805a84ea288384416221.
> > >
> > > When a binary links against a .a archive (as opposed to a shared library),
> > > any symbols which are marked as 'weak' get silently dropped. As a result
> > > when the binary later runs, those 'weak' functions have an address of
> > > 0x0 and thus crash when run.
> > >
> > > This happened with virtlogd and virtlockd because they don't link to
> > > libvirt.so, but instead just libvirt_util.a and libvirt_rpc.a. The
> > > virRandomBits symbols was weak and so left out of the virtlogd &
> > > virtlockd binaries, despite being required by virHashTable functions.
> > >
> > > Various other binaries like libvirt_lxc, libvirt_iohelper, etc also
> > > link directly to .a files instead of libvirt.so, so are potentially
> > > at risk of dropping symbols leading to a later runtime crash.
> > >
> > > This is normal linker behaviour because a weak symbol is not treated
> > > as undefined, so nothing forces it to be pulled in from the .a You
> > > have to force the linker to pull in weak symbols using -u$SYMNAME
> > > which is not a practical approach.
> > >
> >
> > How is this done by gnulib (or libc) when most their functions are weak
> > aliases anyway? Can't we use the same approach they have?
> > virtlo{g,ck}d link with libgnu.la as well and there is no problem with
> > that, right? So I guess this _must_ be solvable somehow, IMHO.
> >
> > I'm just curious how that works.
> >
> > Martin
>
> I guess we would have to do something like the following, but for every
> function.
>
> diff --git i/src/util/virrandom.c w/src/util/virrandom.c
> index 41daa404b273..3d9fe7f85d97 100644
> --- i/src/util/virrandom.c
> +++ w/src/util/virrandom.c
> @@ -99,7 +99,7 @@ VIR_ONCE_GLOBAL_INIT(virRandom)
> *
> * Return: a random number with @nbits entropy
> */
> -uint64_t virRandomBits(int nbits)
> +static uint64_t __virRandomBits(int nbits)
> {
> uint64_t ret = 0;
> int32_t bits;
> @@ -125,6 +125,7 @@ uint64_t virRandomBits(int nbits)
> virMutexUnlock(&randomLock);
> return ret;
> }
> +uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE
> __attribute__((alias("__virRandomBits")));
>
>
> /**
> diff --git i/src/util/virrandom.h w/src/util/virrandom.h
> index 990a456addf7..abda95aef506 100644
> --- i/src/util/virrandom.h
> +++ w/src/util/virrandom.h
> @@ -24,7 +24,7 @@
>
> # include "internal.h"
>
> -uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE;
> +uint64_t virRandomBits(int nbits);
> double virRandom(void);
> uint32_t virRandomInt(uint32_t max);
> int virRandomBytes(unsigned char *buf, size_t buflen)
> --
>
> And of course that could be macrofied so that ATTRIBUTE_MOCKABLE takes
> function or something, etc.
>
> I like this more than reverting the patches.
FYI, I intend to push these revert patches, so that virtlogd stops
crashing to unblock other devs/users, while we focus on writing &
reviewing the new approach we've discussed
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 13, 2017 at 12:28:41PM +0100, Daniel P. Berrange wrote:
On Wed, Jul 12, 2017 at 01:49:04PM +0200, Martin Kletzander wrote:
On Wed, Jul 12, 2017 at 01:10:08PM +0200, Martin Kletzander wrote:
> On Wed, Jul 12, 2017 at 11:14:16AM +0100, Daniel P. Berrange wrote:
> > This reverts commit e4b980c853d2114b25fa805a84ea288384416221.
> >
> > When a binary links against a .a archive (as opposed to a shared library),
> > any symbols which are marked as 'weak' get silently dropped. As a result
> > when the binary later runs, those 'weak' functions have an address of
> > 0x0 and thus crash when run.
> >
> > This happened with virtlogd and virtlockd because they don't link to
> > libvirt.so, but instead just libvirt_util.a and libvirt_rpc.a. The
> > virRandomBits symbols was weak and so left out of the virtlogd &
> > virtlockd binaries, despite being required by virHashTable functions.
> >
> > Various other binaries like libvirt_lxc, libvirt_iohelper, etc also
> > link directly to .a files instead of libvirt.so, so are potentially
> > at risk of dropping symbols leading to a later runtime crash.
> >
> > This is normal linker behaviour because a weak symbol is not treated
> > as undefined, so nothing forces it to be pulled in from the .a You
> > have to force the linker to pull in weak symbols using -u$SYMNAME
> > which is not a practical approach.
> >
>
> How is this done by gnulib (or libc) when most their functions are weak
> aliases anyway? Can't we use the same approach they have?
> virtlo{g,ck}d link with libgnu.la as well and there is no problem with
> that, right? So I guess this _must_ be solvable somehow, IMHO.
>
> I'm just curious how that works.
>
> Martin
I guess we would have to do something like the following, but for every
function.
diff --git i/src/util/virrandom.c w/src/util/virrandom.c
index 41daa404b273..3d9fe7f85d97 100644
--- i/src/util/virrandom.c
+++ w/src/util/virrandom.c
@@ -99,7 +99,7 @@ VIR_ONCE_GLOBAL_INIT(virRandom)
*
* Return: a random number with @nbits entropy
*/
-uint64_t virRandomBits(int nbits)
+static uint64_t __virRandomBits(int nbits)
{
uint64_t ret = 0;
int32_t bits;
@@ -125,6 +125,7 @@ uint64_t virRandomBits(int nbits)
virMutexUnlock(&randomLock);
return ret;
}
+uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE
__attribute__((alias("__virRandomBits")));
/**
diff --git i/src/util/virrandom.h w/src/util/virrandom.h
index 990a456addf7..abda95aef506 100644
--- i/src/util/virrandom.h
+++ w/src/util/virrandom.h
@@ -24,7 +24,7 @@
# include "internal.h"
-uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE;
+uint64_t virRandomBits(int nbits);
double virRandom(void);
uint32_t virRandomInt(uint32_t max);
int virRandomBytes(unsigned char *buf, size_t buflen)
--
And of course that could be macrofied so that ATTRIBUTE_MOCKABLE takes
function or something, etc.
I like this more than reverting the patches.
FYI, I intend to push these revert patches, so that virtlogd stops
crashing to unblock other devs/users, while we focus on writing &
reviewing the new approach we've discussed
Sure, ACK to that.
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
- Original Message -
> On Wed, Jul 12, 2017 at 01:49:04PM +0200, Martin Kletzander wrote:
> > On Wed, Jul 12, 2017 at 01:10:08PM +0200, Martin Kletzander wrote:
> > > On Wed, Jul 12, 2017 at 11:14:16AM +0100, Daniel P. Berrange wrote:
> > > > This reverts commit e4b980c853d2114b25fa805a84ea288384416221.
> > > >
> > > > When a binary links against a .a archive (as opposed to a shared
> > > > library),
> > > > any symbols which are marked as 'weak' get silently dropped. As a
> > > > result
> > > > when the binary later runs, those 'weak' functions have an address of
> > > > 0x0 and thus crash when run.
> > > >
> > > > This happened with virtlogd and virtlockd because they don't link to
> > > > libvirt.so, but instead just libvirt_util.a and libvirt_rpc.a. The
> > > > virRandomBits symbols was weak and so left out of the virtlogd &
> > > > virtlockd binaries, despite being required by virHashTable functions.
> > > >
> > > > Various other binaries like libvirt_lxc, libvirt_iohelper, etc also
> > > > link directly to .a files instead of libvirt.so, so are potentially
> > > > at risk of dropping symbols leading to a later runtime crash.
> > > >
> > > > This is normal linker behaviour because a weak symbol is not treated
> > > > as undefined, so nothing forces it to be pulled in from the .a You
> > > > have to force the linker to pull in weak symbols using -u$SYMNAME
> > > > which is not a practical approach.
> > > >
> > >
> > > How is this done by gnulib (or libc) when most their functions are weak
> > > aliases anyway? Can't we use the same approach they have?
> > > virtlo{g,ck}d link with libgnu.la as well and there is no problem with
> > > that, right? So I guess this _must_ be solvable somehow, IMHO.
> > >
> > > I'm just curious how that works.
> > >
> > > Martin
> >
> > I guess we would have to do something like the following, but for every
> > function.
> >
> > diff --git i/src/util/virrandom.c w/src/util/virrandom.c
> > index 41daa404b273..3d9fe7f85d97 100644
> > --- i/src/util/virrandom.c
> > +++ w/src/util/virrandom.c
> > @@ -99,7 +99,7 @@ VIR_ONCE_GLOBAL_INIT(virRandom)
> > *
> > * Return: a random number with @nbits entropy
> > */
> > -uint64_t virRandomBits(int nbits)
> > +static uint64_t __virRandomBits(int nbits)
> > {
> > uint64_t ret = 0;
> > int32_t bits;
> > @@ -125,6 +125,7 @@ uint64_t virRandomBits(int nbits)
> > virMutexUnlock(&randomLock);
> > return ret;
> > }
> > +uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE
> > __attribute__((alias("__virRandomBits")));
> >
> >
> > /**
> > diff --git i/src/util/virrandom.h w/src/util/virrandom.h
> > index 990a456addf7..abda95aef506 100644
> > --- i/src/util/virrandom.h
> > +++ w/src/util/virrandom.h
> > @@ -24,7 +24,7 @@
> >
> > # include "internal.h"
> >
> > -uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE;
> > +uint64_t virRandomBits(int nbits);
> > double virRandom(void);
> > uint32_t virRandomInt(uint32_t max);
> > int virRandomBytes(unsigned char *buf, size_t buflen)
> > --
> >
> > And of course that could be macrofied so that ATTRIBUTE_MOCKABLE takes
> > function or something, etc.
> >
> > I like this more than reverting the patches.
>
> FYI, I intend to push these revert patches, so that virtlogd stops
> crashing to unblock other devs/users, while we focus on writing &
> reviewing the new approach we've discussed
That's what I do locally atm, so ack
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange
> |:|
> |: https://libvirt.org -o-https://fstop138.berrange.com
> |:|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange
> |:|
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 13, 2017 at 07:52:56AM -0400, Marc-André Lureau wrote:
>
>
> - Original Message -
> >
> > FYI, I intend to push these revert patches, so that virtlogd stops
> > crashing to unblock other devs/users, while we focus on writing &
> > reviewing the new approach we've discussed
>
> That's what I do locally atm, so ack
Ok, pushed now.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/13/2017 04:36 AM, Kothapally Madhu Pavan wrote:
> QEMU fails to launch a sPAPR guest with clock sources other that RTC.
> Internally qemu only uses RTC timer for hwclock. This patch reports
> the right error message instead of qemu erroring out when any other
> timer other than RTC is used.
>
How does it fail exactly? Is it a qemu error message or a guest OS failure?
If it's from qemu, and the error message is reasonably clear what hardware/xml
config option is at fauly, then these checks don't add much functional
benefit, just more code to maintain.
A general point, these types of checks should be considered for
qemuDomainDefValidate which adds the benefit of rejecting the config at XML
define time.
Thanks,
Cole
> Signed-off-by: Kothapally Madhu Pavan
> ---
> src/qemu/qemu_command.c | 28 +++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c53ab97..31561ce 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6440,6 +6440,15 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
> break;
>
> case VIR_DOMAIN_TIMER_NAME_PIT:
> +/* Only RTC timer is supported as hwclock for sPAPR machines */
> +if (ARCH_IS_PPC64(def->os.arch)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported clock timer '%s' for '%s'
> architecture"),
> +
> virDomainTimerNameTypeToString(def->clock.timers[i]->name),
> + virArchToString(def->os.arch));
> +return -1;
> +}
> +
> switch (def->clock.timers[i]->tickpolicy) {
> case -1:
> case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY:
> @@ -6483,13 +6492,21 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
> break;
>
> case VIR_DOMAIN_TIMER_NAME_HPET:
> +/* Only RTC timer is supported as hwclock for sPAPR machines */
> +if (ARCH_IS_PPC64(def->os.arch)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported clock timer '%s' for '%s'
> architecture"),
> +
> virDomainTimerNameTypeToString(def->clock.timers[i]->name),
> + virArchToString(def->os.arch));
> +return -1;
> +}
> +
> /* the only meaningful attribute for hpet is "present". If
> * present is -1, that means it wasn't specified, and
> * should be left at the default for the
> * hypervisor. "default" when -no-hpet exists is "yes",
> * and when -no-hpet doesn't exist is "no". "confusing"?
> * "yes"! */
> -
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_HPET)) {
> if (def->clock.timers[i]->present == 0)
> virCommandAddArg(cmd, "-no-hpet");
> @@ -7047,6 +7064,15 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
> for (i = 0; i < def->clock.ntimers; i++) {
> virDomainTimerDefPtr timer = def->clock.timers[i];
>
> +/* Only RTC timer is supported as hwclock for sPAPR machines */
> +if (ARCH_IS_PPC64(def->os.arch) && timer->name !=
> VIR_DOMAIN_TIMER_NAME_RTC) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported clock timer '%s' for '%s'
> architecture"),
> +
> virDomainTimerNameTypeToString(def->clock.timers[i]->name),
> + virArchToString(def->os.arch));
> +return -1;
> +}
> +
> if (timer->name == VIR_DOMAIN_TIMER_NAME_KVMCLOCK &&
> timer->present != -1) {
> virBufferAsprintf(&buf, "%s,%ckvmclock",
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 13, 2017 at 12:25:50PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 13, 2017 at 01:14:12PM +0200, Martin Kletzander wrote:
On Wed, Jul 12, 2017 at 05:06:22PM +0100, Daniel P. Berrange wrote:
> Currently any mockable functions are marked with attributes
> noinline, noclone and weak. This prevents the compiler from
> optimizing away the impl of these functions.
>
> It has an unfortunate side effect with the libtool convenience
> libraries, if executables directly link to them. For example
> virlockd, virlogd both link to libvirt_util.la When this is
> done, the linker does not consider weak symbols as being
> undefined, so it never copies them into the final executable.
>
> In this new approach, we stop annotating the headers entirely.
> Instead we create a weak function alias in the source.
>
> int fooImpl(void) {
> ..the real code..
> }
>
> int foo(void) __attribute__((noinline, noclone, weak, alias("fooImpl"))
>
> If any functions in the same file call "foo", this prevents the
> optimizer from inlining any part of fooImpl. When linking to the
> libtool convenience static library, we also get all the symbols
> present. Finally the test suite can just directly define a
> 'foo' function in its source, removing the need to use LD_PRELOAD
> (though removal of LD_PRELOADS is left for a future patch).
>
What are you using for tag navigation? With this macro-defined
functions I cannot easily jump to them (the main reason why I don't like
macros defining functions).
Grep & historic knowledge :-)
I would very much prefer if
ATTRIBUTE_MOCKABLE just took a parameter like this:
#define ATTRIBUTE_MOCKABLE(func) __attribute__((noinline, noclone, weak, alias(#func
"Impl"))
(written by hand, don't take mu word for it working out of the box) and
the original function would be left untouched.
Yeah, that is certainly a valid alternative. I was not entirely happy
with the results I get here. As long as we don't mind repeating the
arg list in both places, your approach is more attractive, despite
the duplication.
I think we don't if we do:
#define VIR_MOCKABLE(ret, name) typeof(name ## Impl) name \
__attribute__((noinline, weak, __alias__(#name "Impl")));
or can't this be done for functions? It worked when I tried it now.
I have one more idea, though. So that we don't have to double-type the
actual definition of the Impl function, we can make it static. What if
we make *all* functions static and only add weak aliases to those that
are supposed to be used outside the file? That is after we deal with
the Darwin case, of course.
Also, this fails on OS X [1] and I don't really see why:
util/vircommand.c:988:1: error: only weak aliases are supported on darwin
VIR_MOCKABLE(void,
^
./internal.h:252:60: note: expanded from macro 'VIR_MOCKABLE'
ret name(__VA_ARGS__) __attribute__((noinline, weak, __alias__(#name
"Impl"))); \
It appears clang simply doesn't support 'alias' on Darwin.
Docs suggest that we need to use 'weakref(#name "Impl")' so I'll have
to test whether that's viable for our usage scenario or not.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 13, 2017 at 02:33:57PM +0200, Martin Kletzander wrote:
> On Thu, Jul 13, 2017 at 12:25:50PM +0100, Daniel P. Berrange wrote:
> > On Thu, Jul 13, 2017 at 01:14:12PM +0200, Martin Kletzander wrote:
> > > On Wed, Jul 12, 2017 at 05:06:22PM +0100, Daniel P. Berrange wrote:
> > > > Currently any mockable functions are marked with attributes
> > > > noinline, noclone and weak. This prevents the compiler from
> > > > optimizing away the impl of these functions.
> > > >
> > > > It has an unfortunate side effect with the libtool convenience
> > > > libraries, if executables directly link to them. For example
> > > > virlockd, virlogd both link to libvirt_util.la When this is
> > > > done, the linker does not consider weak symbols as being
> > > > undefined, so it never copies them into the final executable.
> > > >
> > > > In this new approach, we stop annotating the headers entirely.
> > > > Instead we create a weak function alias in the source.
> > > >
> > > > int fooImpl(void) {
> > > > ..the real code..
> > > > }
> > > >
> > > > int foo(void) __attribute__((noinline, noclone, weak,
> > > > alias("fooImpl"))
> > > >
> > > > If any functions in the same file call "foo", this prevents the
> > > > optimizer from inlining any part of fooImpl. When linking to the
> > > > libtool convenience static library, we also get all the symbols
> > > > present. Finally the test suite can just directly define a
> > > > 'foo' function in its source, removing the need to use LD_PRELOAD
> > > > (though removal of LD_PRELOADS is left for a future patch).
> > > >
> > >
> > > What are you using for tag navigation? With this macro-defined
> > > functions I cannot easily jump to them (the main reason why I don't like
> > > macros defining functions).
> >
> > Grep & historic knowledge :-)
> >
> > > I would very much prefer if
> > > ATTRIBUTE_MOCKABLE just took a parameter like this:
> > >
> > > #define ATTRIBUTE_MOCKABLE(func) __attribute__((noinline, noclone, weak,
> > > alias(#func "Impl"))
> > >
> > > (written by hand, don't take mu word for it working out of the box) and
> > > the original function would be left untouched.
> >
> > Yeah, that is certainly a valid alternative. I was not entirely happy
> > with the results I get here. As long as we don't mind repeating the
> > arg list in both places, your approach is more attractive, despite
> > the duplication.
> >
>
> I think we don't if we do:
>
> #define VIR_MOCKABLE(ret, name) typeof(name ## Impl) name \
>__attribute__((noinline, weak, __alias__(#name "Impl")));
>
> or can't this be done for functions? It worked when I tried it now.
I've never tried typeof in this way, so I guess we can experiment.
> I have one more idea, though. So that we don't have to double-type the
> actual definition of the Impl function, we can make it static. What if
> we make *all* functions static and only add weak aliases to those that
> are supposed to be used outside the file? That is after we deal with
> the Darwin case, of course.
NB, I orignally marked the funtions as static, but clang complains that
they are unused, despite being referenced from an alias annotation.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/13/2017 05:49 PM, Cole Robinson wrote:
On 07/13/2017 04:36 AM, Kothapally Madhu Pavan wrote:
QEMU fails to launch a sPAPR guest with clock sources other that RTC.
Internally qemu only uses RTC timer for hwclock. This patch reports
the right error message instead of qemu erroring out when any other
timer other than RTC is used.
How does it fail exactly? Is it a qemu error message or a guest OS failure?
If it's from qemu, and the error message is reasonably clear what hardware/xml
config option is at fauly, then these checks don't add much functional
benefit, just more code to maintain.
If it's from qemu, and the error message is reasonably clear what hardware/xml
config option is at fauly, then these checks don't add much functional
benefit, just more code to maintain.
When we use kvmclock timer in domain xml as:
Domain fails to start with following error:
#virsh start --console virt-tests-vm1
error: Failed to start domain virt-tests-vm1
error: internal error: process exited while connecting to monitor:
2017-04-25T09:31:58.180062Z qemu-system-ppc64: Unable to find CPU definition:
qemu64
This is because the qemu cpu command line generated when kvmclock
timer is used is:
-cpu qemu64,+kvmclock
This happens because in qemuBuildCpuCommandLine has default_model = qemu64,
When I corrected the default model to "host" for ppc64 machine,
qemu cpu commandline generated is:
-cpu host,+kvmclock
This is a valid qemu command for ppc64 machine. Now the qemu
fails to start with folloeing error:
qemu-system-ppc64: Expected key=value format, found +kvmclock.
Similarly when kvm-pit timer is used qemu warns as below:
sudo ./qemu-system-ppc64 -name migrate_qemu -boot strict=on -device
nec-usb-xhci,id=usb,bus=pci.0,addr=0xf -device spapr-vscsi,id=scsi0,reg=0x2000
-smp 1,maxcpus=4,sockets=4,cores=1,threads=1 --machine
pseries,accel=kvm,kvm-type=HV,usb=off,dump-guest-core=off -m
4G,slots=32,maxmem=32G -drive
file=/home/danielhb/vm_imgs/ubuntu1704.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=none
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x2,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
-nographic -global kvm-pit.lost_tick_policy=discard
qemu-system-ppc64: Warning: global kvm-pit.lost_tick_policy has invalid class
name
Basically, RTC is the only valid clocksource for sPAPR guests. For other clock
sources
qemu either errors out or internally considers RTC as default.
A general point, these types of checks should be considered for
qemuDomainDefValidate which adds the benefit of rejecting the config at XML
define time.
I was of the opinion, the existing the domain definitions would fail to
be parsed if I add
in qemuDomainDefValidate(). Now, while I reply to you I realise, there
is no way someone
would have attempted to use non-RTC clock sources. So, its perfectly
safe to move these
checks to qemuDomainDefValidate().
Will attempt it in V2.
Thanks,
Cole
Signed-off-by: Kothapally Madhu Pavan
---
src/qemu/qemu_command.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c53ab97..31561ce 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6440,6 +6440,15 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
break;
case VIR_DOMAIN_TIMER_NAME_PIT:
+/* Only RTC timer is supported as hwclock for sPAPR machines */
+if (ARCH_IS_PPC64(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported clock timer '%s' for '%s'
architecture"),
+
virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+ virArchToString(def->os.arch));
+return -1;
+}
+
switch (def->clock.timers[i]->tickpolicy) {
case -1:
case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY:
@@ -6483,13 +6492,21 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
break;
case VIR_DOMAIN_TIMER_NAME_HPET:
+/* Only RTC timer is supported as hwclock for sPAPR machines */
+if (ARCH_IS_PPC64(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported clock timer '%s' for '%s'
architecture"),
+
virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+ virArchToString(def->os.arch));
+return -1;
+}
+
/* the only meaningful attribute for hpet is "present". If
* present is -1, that means it wasn't specified, and
* should be left at the default for the
* hypervisor. "default" when -no-hpet exists is "yes",
* and when -no-hpet doesn't exist is "no". "confusing"?
* "yes"! */
-
On 07/13/2017 09:52 AM, Madhu Pavan wrote:
>
>
> On 07/13/2017 05:49 PM, Cole Robinson wrote:
>> On 07/13/2017 04:36 AM, Kothapally Madhu Pavan wrote:
>>> QEMU fails to launch a sPAPR guest with clock sources other that RTC.
>>> Internally qemu only uses RTC timer for hwclock. This patch reports
>>> the right error message instead of qemu erroring out when any other
>>> timer other than RTC is used.
>>>
>> How does it fail exactly? Is it a qemu error message or a guest OS failure?
>>
>> If it's from qemu, and the error message is reasonably clear what
>> hardware/xml
>> config option is at fauly, then these checks don't add much functional
>> benefit, just more code to maintain.
>
> If it's from qemu, and the error message is reasonably clear what hardware/xml
> config option is at fauly, then these checks don't add much functional
> benefit, just more code to maintain.
>
> When we use kvmclock timer in domain xml as:
>
>
>
> Domain fails to start with following error:
> #virsh start --console virt-tests-vm1
> error: Failed to start domain virt-tests-vm1
> error: internal error: process exited while connecting to monitor:
> 2017-04-25T09:31:58.180062Z qemu-system-ppc64: Unable to find CPU definition:
> qemu64
>
> This is because the qemu cpu command line generated when kvmclock
> timer is used is:
> -cpu qemu64,+kvmclock
> This happens because in qemuBuildCpuCommandLine has default_model = qemu64,
> When I corrected the default model to "host" for ppc64 machine,
> qemu cpu commandline generated is:
> -cpu host,+kvmclock
> This is a valid qemu command for ppc64 machine. Now the qemu
> fails to start with folloeing error:
> qemu-system-ppc64: Expected key=value format, found +kvmclock.
>
Hmm, well that qemu64 bit seems like something else to fix, but not a blocker
for this.
> Similarly when kvm-pit timer is used qemu warns as below:
> sudo ./qemu-system-ppc64 -name migrate_qemu -boot strict=on -device
> nec-usb-xhci,id=usb,bus=pci.0,addr=0xf -device spapr-vscsi,id=scsi0,reg=0x2000
> -smp 1,maxcpus=4,sockets=4,cores=1,threads=1 --machine
> pseries,accel=kvm,kvm-type=HV,usb=off,dump-guest-core=off -m
> 4G,slots=32,maxmem=32G -drive
> file=/home/danielhb/vm_imgs/ubuntu1704.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=none
> -device
> virtio-blk-pci,scsi=off,bus=pci.0,addr=0x2,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
> -nographic -global kvm-pit.lost_tick_policy=discard
>
> qemu-system-ppc64: Warning: global kvm-pit.lost_tick_policy has invalid class
> name
>
> Basically, RTC is the only valid clocksource for sPAPR guests. For other clock
> sources
> qemu either errors out or internally considers RTC as default.
>
If qemu just prints a warning in that case, then I agree we should explicitly
reject it in libvirt. Though that does mean that guests that were possibly
working before, but with a qemu warning, will now fail to redefine. Not sure
we care enough for this case though
>>
>> A general point, these types of checks should be considered for
>> qemuDomainDefValidate which adds the benefit of rejecting the config at XML
>> define time.
> I was of the opinion, the existing the domain definitions would fail to be
> parsed if I add
> in qemuDomainDefValidate(). Now, while I reply to you I realise, there is no
> way someone
> would have attempted to use non-RTC clock sources. So, its perfectly safe to
> move these
> checks to qemuDomainDefValidate().
>
> Will attempt it in V2.
>
That was the case at one point, but nowadays this is skipped when reading XML
from disk, see the VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE flag.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> Now that we have a bit more control, let's convert our object into a
> recursive lockable object and let that magic handle the create and
> lock/unlock.
>
> Additionally, we introduce virNWFilterObjEndAPI.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virnwfilterobj.c | 127
> ++---
> src/conf/virnwfilterobj.h | 6 --
> src/libvirt_private.syms | 2 -
> src/nwfilter/nwfilter_driver.c | 4 +-
> src/nwfilter/nwfilter_gentech_driver.c | 10 +--
> 5 files changed, 44 insertions(+), 105 deletions(-)
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> Rename to virNWFilterDoInstantiate to better describe the action.
>
> Also fix the @vmuuid parameter to not have the ATTRIBUTE_UNUSED since it
> is used in the call to virNWFilterDHCPSnoopReq.
>
> Signed-off-by: John Ferlan
> ---
> src/nwfilter/nwfilter_gentech_driver.c | 46
> +++---
> 1 file changed, 20 insertions(+), 26 deletions(-)
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> Modify the virNWFilterObjNew to take @def as a parameter and consume it.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virnwfilterobj.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
As I've stated in the other review for secret patch set, I don't see
much value in this patch. What your reasoning for it? We don't usually
do it this way.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> The current algorithm required usage of recursive locks because there
> was no other mechanism available to ensure that the object wasn't deleted
> whilst the instantiation was taking place.
>
> Now that we have object references, lets use those to ensure the object
> isn't deleted whilst we're working through the instantiation thus removing
> the need for recursive locks.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virnwfilterobj.c | 13 +--
> src/nwfilter/nwfilter_gentech_driver.c | 40
> +++---
> 2 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index 84ef7d1..ea1323d 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -303,13 +303,22 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr
> nwfilters,
> }
>
>
> +/**
> + * To avoid the need to have recursive locks as a result of how the
> + * virNWFilterInstantiateFilter processing works, this API will not
> + * lock the returned object, instead just increase the refcnt of the
> + * object to the caller to allow the caller to handle.
> + */
> virNWFilterObjPtr
> virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
> const char *filtername)
> {
> virNWFilterObjPtr obj;
>
> -if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) {
> +virObjectLock(nwfilters);
> +obj = virNWFilterObjListFindByNameLocked(nwfilters, filtername);
> +virObjectUnlock(nwfilters);
> +if (!obj) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("referenced filter '%s' is missing"), filtername);
> return NULL;
> @@ -318,7 +327,7 @@
> virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
> if (virNWFilterObjWantRemoved(obj)) {
> virReportError(VIR_ERR_NO_NWFILTER,
> _("Filter '%s' is in use."), filtername);
> -virNWFilterObjEndAPI(&obj);
> +virNWFilterObjUnref(obj);
> return NULL;
> }
So now an unlocked obj is returned. This feels wrong ... So for
instance: virNWFilterIncludeDefToRuleInst() calls
virNWFilterObjListFindInstantiateFilter(). So it obtains ref'd but
unlocked object. Then it calls virNWFilterObjGetDef() over it. Well, we
are reading without lock, so we might as well be accessing stale
pointer. For instance the following might happen:
1) threadA is in virNWFilterIncludeDefToRuleInst() and calls
virNWFilterObjGetDef(). It gets pointer to current definition of nwfilter.
2) threadB starts to update the definition of the object. Since the
object is not locked, nothing stops it from doing so. As part of the
process, current definition is freed.
3) threadA touches freed data.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> Rename to virNWFilterInstantiateFilterUpdate and alter the callers to not
> have one parameter per line.
>
> Signed-off-by: John Ferlan
> ---
> src/nwfilter/nwfilter_gentech_driver.c | 62
> +-
> 1 file changed, 24 insertions(+), 38 deletions(-)
ACK
BTW I'm curious why the "Update" suffix?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> After returning from virNWFilterObjListAssignDef the @obj is locked;
> however, if virNWFilterSaveConfig fails to save the generated UUID
> the code jumped to error and returns NULL meaning the caller will
> not call virNWFilterObjUnlock on the object leaving the object
> locked on list and ripe for a deadlock on any subsequent Find
> of all objects or object removal.
>
> So rather than jumping to error alter the comment prior to the
> virNWFilterSaveConfig and just provide a VIR_INFO message for anyone
> that cares, but allow the code to continue and a subsequent LoadConfig
> to once again attempt the save of a newly generated UUID.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virnwfilterobj.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index 3fb6046..0343c0a 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -507,10 +507,13 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr
> nwfilters,
> def = NULL;
> objdef = obj->def;
>
> -/* We generated a UUID, make it permanent by saving the config to disk */
> +/* We generated a UUID, atttempt to make it permanent by saving the
s/ttt/tt/
> + * config to disk. If not successful, no need to fail or remove the
> + * object as a future load would regenerate a UUID and try again,
> + * but the existing config would still exist and can be used. */
> if (!objdef->uuid_specified &&
> virNWFilterSaveConfig(configDir, objdef) < 0)
> -goto error;
> +VIR_INFO("failed to save generated UUID for filter '%s'",
> objdef->name);
>
> VIR_FREE(configFile);
> return obj;
>
Well, frankly I'd say that we should not even try to save the config in
the first place. Load() should really just load. We shouldn't try to
"fix" XML configs at load time rather then when saving it in define phase.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> Use @nwfilter always for the name
>
> Signed-off-by: John Ferlan
> ---
> src/nwfilter/nwfilter_driver.c | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)
Okay, not much value, but at least we are consistent now :-)
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> When creating an object, save the configFile name in the object rather
> than needing to build it up each time for the SaveConfig. This involves
> adding a @configDir parameter to virNWFilterObjListAssignDef and removing
> the @configDir from virNWFilterObjSaveConfig.
Why? This keeps the path in memory for the whole life time of the object
even though it's needed just occasionally - at the beginning and
probably at the end.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virnwfilterobj.c | 38 --
> src/conf/virnwfilterobj.h | 6 +++---
> src/nwfilter/nwfilter_driver.c | 5 +++--
> 3 files changed, 26 insertions(+), 23 deletions(-)
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> New API will be virNWFilterInstantiateFilterInternal as it's called from
> the virNWFilterInstantiateFilter and virNWFilterUpdateInstantiateFilter.
>
> Signed-off-by: John Ferlan
> ---
> src/nwfilter/nwfilter_gentech_driver.c | 28 ++--
> 1 file changed, 14 insertions(+), 14 deletions(-)
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> If the virNWFilterSaveConfig in virNWFilterObjListLoadConfig, then jumping
s/,/ fails,/
> to the error label would free the @def owned by the object, but the object
> is still on the list.
>
> Fix this by following proper procedure to clear @def and create a new
> variable @objdef to handle the object's def after successful assignment.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virnwfilterobj.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index 0027d45..3fb6046 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -485,6 +485,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr
> nwfilters,
> {
> virNWFilterDefPtr def = NULL;
> virNWFilterObjPtr obj;
> +virNWFilterDefPtr objdef;
> char *configFile = NULL;
>
> if (!(configFile = virFileBuildPath(configDir, name, ".xml")))
> @@ -503,10 +504,12 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr
> nwfilters,
>
> if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
> goto error;
> +def = NULL;
> +objdef = obj->def;
>
> /* We generated a UUID, make it permanent by saving the config to disk */
> -if (!def->uuid_specified &&
> -virNWFilterSaveConfig(configDir, def) < 0)
> +if (!objdef->uuid_specified &&
> +virNWFilterSaveConfig(configDir, objdef) < 0)
> goto error;
This @objdef variable looks redundant to me. Can't we access obj->def
directly? Esp. since you're introducing a variable just for a two times
use. Then again, we often use obj->def->... when it comes to domain
objects, why not here?
ACK if you drop the @objdef variable.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/02/2017 12:25 PM, John Ferlan wrote:
> No need to goto cleanup and check "if (obj)" if we know (obj) isn't there,
> so just return immediately.
>
> Signed-off-by: John Ferlan
> ---
> src/nwfilter/nwfilter_driver.c | 15 ++-
> 1 file changed, 6 insertions(+), 9 deletions(-)
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Jul 02, 2017 at 04:16:02AM +0200, Marek Marczykowski-Górecki wrote:
> On Sun, Feb 26, 2017 at 07:02:24PM -0700, Jim Fehlig wrote:
> > Long ago danpb posted some patches to test libvirt domXML to
> > libxl_domain_config conversion
> >
> > https://www.redhat.com/archives/libvir-list/2014-May/msg01102.html
> >
> > Some of the prerequisite patches were pushed, but we've never managed
> > to push patches actually providing the conversion tests. I sent several
> > follow-ups to Dan's work but never converged on a satisfactory solution
> > for all the Xen versions supported by libvirt. The last attempt was in
> > Sept 2014
> >
> > https://www.redhat.com/archives/libvir-list/2014-September/msg00698.html
> >
> > I tried to revive the work in Jan 2015, but that also stalled
> >
> > https://www.redhat.com/archives/libvir-list/2015-January/msg00924.html
> >
> > Fast-forward over 2.5 years from the first attempt and libvirt no longer
> > supports older Xen versions 4.2 and 4.3 that were proving to be problematic.
> > Starting with Xen 4.5 libxl added support for libxl_domain_config_from_json,
> > which provides a way to implement the conversion tests that work with all
> > Xen versions >= 4.5 (including latest xen.git master).
>
> Few more months have passed...
>
> FWIW, I've tested it with Xen 4.6. The patch needs very minor update:
> - s/VIRT_TEST_MAIN_PRELOAD/VIR_TEST_MAIN_PRELOAD/
> - add xencaps argument to libxlBuildDomainConfig call
>
> After that, it works! When I made some test to fail, reported error is
> not so helpful ("libvirt: Xen Light Driver error : internal error:
> Expected and actual libxl_domain_config objects do not compare"), but it
> do catch failures.
> Then, if I change strcmp to virTestCompareToString, the output is much
> more helpful.
>
> I'd really love to have it merged, mostly because I want to add more tests
> using this framework (see "Add setting CPU features (CPUID) with
> libxenlight driver" thread).
> Is there anything I can do to make it happen?
Ping?
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Make use of an error: label to handle the failure and need to call
>> virSecretObjEndAPI for the object to set it to NULL for return.
>>
>> Also rather than an if/else processing - have the initial "if" which
>> is just replacing the @newdef into obj->def goto cleanup, thus allowing
>> the remaining code to be extracted from the else and appear to more inline.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virsecretobj.c | 74
>> -
>> 1 file changed, 37 insertions(+), 37 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index e3bcbe5..1bafd0b 100644
>> --- a/src/conf/virsecretobj.c
>> +++ b/src/conf/virsecretobj.c
>> @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> {
>> virSecretObjPtr obj;
>> virSecretDefPtr objdef;
>> -virSecretObjPtr ret = NULL;
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> char *configFile = NULL, *base64File = NULL;
>>
>> @@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> _("a secret with UUID %s is already defined for "
>> "use with %s"),
>> uuidstr, objdef->usage_id);
>> -goto cleanup;
>> +goto error;
>> }
>>
>> if (objdef->isprivate && !newdef->isprivate) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("cannot change private flag on existing
>> secret"));
>> -goto cleanup;
>> +goto error;
>> }
>>
>> if (oldDef)
>> @@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> else
>> virSecretDefFree(objdef);
>> obj->def = newdef;
>> -} else {
>> -/* No existing secret with same UUID,
>> - * try look for matching usage instead */
>> -if ((obj = virSecretObjListFindByUsageLocked(secrets,
>> - newdef->usage_type,
>> - newdef->usage_id))) {
>> -virObjectLock(obj);
>> -objdef = obj->def;
>> -virUUIDFormat(objdef->uuid, uuidstr);
>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("a secret with UUID %s already defined for "
>> - "use with %s"),
>> - uuidstr, newdef->usage_id);
>> -goto cleanup;
>> -}
>> +goto cleanup;
>> +}
>>
>> -/* Generate the possible configFile and base64File strings
>> - * using the configDir, uuidstr, and appropriate suffix
>> - */
>> -if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
>> -!(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
>> -goto cleanup;
>> +/* No existing secret with same UUID,
>> + * try to look for matching usage instead */
>> +if ((obj = virSecretObjListFindByUsageLocked(secrets,
>> + newdef->usage_type,
>> + newdef->usage_id))) {
>> +virObjectLock(obj);
>> +objdef = obj->def;
>> +virUUIDFormat(objdef->uuid, uuidstr);
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("a secret with UUID %s already defined for "
>> + "use with %s"),
>> + uuidstr, newdef->usage_id);
>> +goto error;
>> +}
>>
>> -if (!(obj = virSecretObjNew()))
>> -goto cleanup;
>> +/* Generate the possible configFile and base64File strings
>> + * using the configDir, uuidstr, and appropriate suffix
>> + */
>> +if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
>> +!(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
>> +goto cleanup;
>>
>> -if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
>> -goto cleanup;
>> +if (!(obj = virSecretObjNew()))
>> +goto cleanup;
>>
>> -obj->def = newdef;
>> -VIR_STEAL_PTR(obj->configFile, configFile);
>> -VIR_STEAL_PTR(obj->base64File, base64File);
>> -virObjectRef(obj);
>> -}
>> +if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
>> +goto error;
>
> Frankly, I find this very confusing. While reading the commit message, I
> understand why you sometimes jump to cleanup and other times to error
> label. But if I were to just look at the code alone, it's completely
> random to me. I think I'd much rather see the pattern that's here.
> However, if you really dislike if-else branching (dislike also as in
> prepare for upcoming patches), I suggest changing just that.
>
> Mic
On Tue, Jul 04, 2017 at 05:03:43AM +0200, Marek Marczykowski-Górecki wrote:
> Tests (patches 3 and 4) depends on libxl_domain_config test suite:
> https://www.redhat.com/archives/libvir-list/2017-February/msg01477.html
>
> But first two patches can be applied independently.
Anything missing here? If test suite cannot be merged for any reason,
could you at least merge the functionality itself?
> Marek Marczykowski-Górecki (4):
> cpu: define sub-leaf 0 for leaf 7 in cpu_map.xml
> libxl: add support for CPUID features policy
> tests: switch libxlxml2domconfig test to use testXLInintCaps
> tests: check CPU features handling in libxl driver
>
> src/cpu/cpu_map.xml | 58 ++---
> src/libxl/libxl_conf.c | 77 -
> src/libxl/libxl_conf.h | 1 +-
> tests/libxlxml2domconfigdata/basic-pv.xml| 2 +-
> tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 ++-
> tests/libxlxml2domconfigdata/fullvirt-cpuid.xml | 37 -
> tests/libxlxml2domconfigtest.c | 3 +-
> 7 files changed, 207 insertions(+), 35 deletions(-)
> create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json
> create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
>
> base-commit: fbcc08b245bb7d5502633d9cb4048281cc9d8532
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Move the consumption of @newdef into virSecretObjNew and then handle that
>> in the calling path. Because on error the calling code expects to free
>> @newdef, unset obj->def for the creation failure error paths.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virsecretobj.c | 14 +-
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index c0bcfab..ca13cf8 100644
>> --- a/src/conf/virsecretobj.c
>> +++ b/src/conf/virsecretobj.c
>> @@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
>> VIR_ONCE_GLOBAL_INIT(virSecretObj)
>>
>> static virSecretObjPtr
>> -virSecretObjNew(void)
>> +virSecretObjNew(virSecretDefPtr def)
>> {
>> virSecretObjPtr obj;
>>
>> @@ -98,6 +98,7 @@ virSecretObjNew(void)
>> return NULL;
>>
>> virObjectLock(obj);
>> +obj->def = def;
>>
>> return obj;
>> }
>> @@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> goto error;
>> }
>>
>> -if (!(obj = virSecretObjNew()))
>> +if (!(obj = virSecretObjNew(newdef)))
>> goto cleanup;
>>
>> /* Generate the possible configFile and base64File strings
>> * using the configDir, uuidstr, and appropriate suffix
>> */
>> if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
>> -!(obj->base64File = virFileBuildPath(configDir, uuidstr,
>> ".base64")))
>> +!(obj->base64File = virFileBuildPath(configDir, uuidstr,
>> ".base64"))) {
>> +obj->def = NULL;
>> goto error;
>> +}
>
> I don't quite see the value of this patch, esp. because you have to
> manually unset the ->def in each error path.
>
> Michal
>
Well that's part of that "longer term" vision thing where I was having
the @def be consumed in a new object. I've had to scale that back a bit
due to comments related to the object, but this code was already was all
being done in parallel - so that's why it's like that.
I could drop this one, although having @def consumed by vir*ObjNew() is
something that I have been doing throughout the various changes. So
far, virInterfaceObjNew already has this, but patches for nwfilter and
nodedev also follow the same pattern.
I'm 50/50 right now on it and can drop it if you'd prefer. Yes, the
drawback is "obvious" that on failure, clearing obj->def needs to be
done to avoid the potential double free problem.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Since the virSecretObjListAdd technically consumes @def on success,
>> the secretDefineXML should fetch the @def from the object afterwards
>> and manage as an @objdef and set @def = NULL immediately.
>
> Really? virSecretObjListAdd sets ->def pointer in the object, but it
> doesn't touch the definition otherwise. So I think a caller is safe to
> continue using the pointer.
>
> Michal
>
Let's consider the code before my change...
@def is added to the @obj
"Something" causes us to jump to the "restore_backup:" label and @backup
== NULL.
That means virSecretObjListRemove runs and because @obj has @def it ends
up calling virSecretDefFree
The very next thing we do is call virSecretDefFree on the same @def address.
While it is the same thing and I could do a VIR_STEAL_PTR(objdef, def);
instead, the patch just follows the pattern I've adopted in other
patches where @def = NULL and objdef = vir*ObjGetDef()
IDC either way, but to avoid a path where @def could be free'd twice,
something needs to be done.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hello all,
I would start to work on adding z/vm hypervisor to libvirt. I am currently
going through libvirt source code and found that I need to add a new driver
implementation for z/VM hypervisor. Can you please let me know the best way
to add a driver to libvirt. Basically looking for a head start on which
part of the code look for ? and any related docs ?
Regards,
*Venkata Krishna Rohit Sakala*
B.Tech 4th Year,
International Institute of Information Technology Hyderabad, India.
Mobile:
*+91-9666255517*
Website: https://researchweb.iiit.ac.in/~rohit.sakala/
ᐧ
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
>> after the call virSecretObjListRemove, be more explicit by calling
>> virObjectUnref and setting @obj to NULL for secretUndefine and in
>> the error path of secretDefineXML.
>>
>> This also fixes a leak during virSecretLoad if the virSecretLoadValue
>> fails the code jumps to cleanup without setting @ret = obj, thus calling
>> virSecretObjListRemove which only accounts for the object reference
>> related to adding the object to the list during virSecretObjListAdd,
>> but does not account for the reference to the object itself as the
>> return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
>> on the object recently added thus reducing the refcnt to zero. Thus
>> cleaning up the virSecretLoadValue error path to make it clearer what
>> needs to be done on failure.
>
> I think the real reason is that we cannot call virSecretObjEndAPI()
> because that *unlocks* the secret object. However, the object is already
> unlocked at that point by virSecretObjListRemove() and thus we would
> unlock twice while locking just once. Honestly, I'd rather see that
> explanation in the commit message. But it's your call.
>
Partially true - although calling Unlock on something already Unlocked
by the same thread IIRC doesn't do much other than cause an error, but
we don't fail on that error.
It's ironic this relates to something Erik and I discussed during the
virNodeDev* changes w/r/t the "owner" (driver) after the Add/Append now
has a reference on the object and would always need to Unref it even
after removing it from the List.
This just happened to be where I had that oh sh*t moment and realized
that when calling Remove we were essentially unlocking and thus yes
calling EndAPI would unlock and unlocked object. I'll add something in
about that..
John
As an aside, this is exactly why I started down the path of common
objects. Consider how the callers to virDomainObjListAdd go through
great lengths to managed the returned object some quite differently
based on what they know about how many refs are returned and whether the
calling function calls virDomainObjEndAPI. If it does, there's always a
virObjectRef done on the object after the Add returns. It's a subtle
thing, but confusing nonetheless.
The thing is the *Remove API will call virHashRemoveEntry which
decrements a refcnt for each table; however, the *Add API only
increments the refcnt once for adding into each table. Callers are very
careful to understand and manage that.
I'd rather see that mgmt go away. It should be "safe" to call EndAPI obj
the @obj regardless if Add or Lookup was used. The callers shouldn't
have to know they need to either use *EndAPI or ObjUnlock. In any case,
I digress and that's a different issue for another day
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virsecretobj.c| 14 ++
>> src/secret/secret_driver.c | 9 +++--
>> 2 files changed, 13 insertions(+), 10 deletions(-)
>
> ACK
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Sorry this keeps falling off the radar...
When posting a v2/v3/vX, please use [PATCH v2] prefix, and make each new
posting a top level patch
On 06/22/2017 08:44 PM, ar...@gnome.org wrote:
> From: Alberto Ruiz
>
> Fixes #913446
>
Full bug link makes things slightly easier if the reviewer wants to look
In the commit message [lease at least give an example of the added XML values,
possibly what dnsmasq value it maps to. Makes grepping commit logs easier
> This patch addresses a few problems found by the initial reviews:
>
> * leaseTimeUnit RNG type renamed to timeUnit
> * virNetworkDHCPDefGetLeaseTime() renamed to virNetworkDHCPLeaseTimeParseXML()
> * consistent use of braces in if-else-if
> * use %lu instead of PRId64
> * use 0 as infinite lease
> * add a leasetime_defined field to struct _virNetworkIPDef to describe
> whether the value was set in the xml configuration or not
> * use uint32_t for the leasetime instead of int64_t
> * fail on all invalid leasetime values
> * squash all patches into one
These types of bits should go after the --- break below: they don't need to be
in the commit message but they are useful for reviewers
There's lots of style issues: please run 'make syntax-check' and fix the
warnings. Peter pointed out some of them already against your 6/21 posting but
they weren't addressed in this version:
https://www.redhat.com/archives/libvir-list/2017-June/msg00838.html
Drop the strcmp usage, we have a STREQ macro we use ('make syntax-check' might
warn but I'm not positive)
CC me on the v3 and I'll do a full review
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 04:27 AM, Pavel Hrdina wrote:
> On Tue, May 09, 2017 at 11:30:09AM -0400, John Ferlan wrote:
>> A virStoragePoolObjPtr will be an 'obj'.
>>
>> A virStoragePoolPtr will be a 'pool'.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/test/test_driver.c | 443
>> -
>> 1 file changed, 219 insertions(+), 224 deletions(-)
>>
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index 548f318..c0e46af 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>
> [...]
>
>> @@ -4057,18 +4056,18 @@ testStoragePoolLookupByUUID(virConnectPtr conn,
>> const unsigned char *uuid)
>> {
>> testDriverPtr privconn = conn->privateData;
>> -virStoragePoolObjPtr pool;
>> +virStoragePoolObjPtr obj;
>> virStoragePoolPtr ret = NULL;
>
> There you didn't changed the "ret" to "pool".
>
>> -if (!(pool = testStoragePoolObjFindByUUID(privconn, uuid)))
>> +if (!(obj = testStoragePoolObjFindByUUID(privconn, uuid)))
>> goto cleanup;
>>
>> -ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid,
>> +ret = virGetStoragePool(conn, obj->def->name, obj->def->uuid,
>> NULL, NULL);
>>
>> cleanup:
>> -if (pool)
>> -virStoragePoolObjUnlock(pool);
>> +if (obj)
>> +virStoragePoolObjUnlock(obj);
>> return ret;
>> }
>>
>> @@ -4078,18 +4077,18 @@ testStoragePoolLookupByName(virConnectPtr conn,
>> const char *name)
>> {
>> testDriverPtr privconn = conn->privateData;
>> -virStoragePoolObjPtr pool;
>> +virStoragePoolObjPtr obj;
>> virStoragePoolPtr ret = NULL;
>
> Same here.
>
>> -if (!(pool = testStoragePoolObjFindByName(privconn, name)))
>> +if (!(obj = testStoragePoolObjFindByName(privconn, name)))
>> goto cleanup;
>>
>> -ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid,
>> +ret = virGetStoragePool(conn, obj->def->name, obj->def->uuid,
>> NULL, NULL);
>>
>> cleanup:
>> -if (pool)
>> -virStoragePoolObjUnlock(pool);
>> +if (obj)
>> +virStoragePoolObjUnlock(obj);
>> return ret;
>> }
>>
>
> [...]
>
>> @@ -4345,7 +4344,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
>> {
>> testDriverPtr privconn = conn->privateData;
>> virStoragePoolDefPtr def;
>> -virStoragePoolObjPtr pool = NULL;
>> +virStoragePoolObjPtr obj = NULL;
>> virStoragePoolPtr ret = NULL;
>
> And here.
>
>> virObjectEventPtr event = NULL;
>>
>
> [...]
>
>> @@ -4419,7 +4418,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
>> {
>> testDriverPtr privconn = conn->privateData;
>> virStoragePoolDefPtr def;
>> -virStoragePoolObjPtr pool = NULL;
>> +virStoragePoolObjPtr obj = NULL;
>> virStoragePoolPtr ret = NULL;
>
> And here
>
I can adjust those... I was more focused on the "existing" @pool and
@obj variables and didn't consider the existing @ret variables.
>> virObjectEventPtr event = NULL;
>>
>
> [...]
>
> I don't like these type of patches. The value to noise ration is poor.
> I'm hesitating to give this patch an ACK even though I probably done
> that in the past.
>
> Pavel
>
So while I understand the concern, my argument becomes is it technically
incorrect to change the names? Secondary to that if you're considering
multiple driver/vir*obj changes at one time, it is *far easier* to
consider an "@obj" to be that thing in the list vir*obj.c list rather
than what is either passed or gets returned from/to the consumer - a
@pool. And yes, technically a @pool is an object - I know.
The second thing that gets fixed by these changes is inconsistent usage.
For instance, prior to these changes look at testParseStorage and
testOpenVolumesForPool. The former uses @obj for a virStoragePoolObjPtr
while the latter uses @pool for a virStoragePoolObjPtr. It becomes
difficult to "follow" code with inconsistencies. So if someone takes the
effort to make things consistent - I think that's better in the long
run. Makes the code more maintainable for a reader, but yes a hassle for
someone back porting some subsequent change because of the merge conflict.
The make code more consistent wins the argument my left and right brain
have over this.
If there's not an ACK, then so be it - I can remove it, but it probably
affects subsequent patches too.
Thanks for going back to earlier series - I had partially given up on
storage and network instead focusing more on secret, nodedev, interface,
and nwfilter. I figured once I got those accepted, I could go back to
storage and network later. This just reduces that workload ;-)
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 04:33 AM, Pavel Hrdina wrote:
> On Tue, May 09, 2017 at 11:30:10AM -0400, John Ferlan wrote:
>> Rework some of the test driver API's to remove the need to return failure
>> when testStoragePoolObjFindByName returns NULL rather than going to cleanup.
>> This removes the need for check for "if (obj)" and in some instances the
>> need to for a cleanup label and a local ret variable.
>
> The recommended wrapping of commit message body is 72 columns.
>
I must violate that a lot ;-)
I adjusted...
John
>> Signed-off-by: John Ferlan
>> ---
>> src/test/test_driver.c | 49
>> -
>> 1 file changed, 16 insertions(+), 33 deletions(-)
>
> Reviewed-by: Pavel Hrdina
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jul 10, 2017 at 12:23 PM, John Ferlan wrote:
>
>
> On 06/28/2017 12:02 PM, dann frazier wrote:
>> Add a path for UEFI VMs for AArch32 VMs. This is the path Debian is
>> currently using in experimental. libvirt is the de facto canonical
>
> "experimental"?
Hi John!
experimental is a staging archive for Debian. I uploaded it there
because Debian was in freeze at the time, not because we're continuing
to weigh options.
> Is this written in stone some where yet?
I'm not sure what qualifies as stone here. I've planted a flag via
Debian, and tried to build consensus on the cross-distro list:
https://lists.linaro.org/pipermail/cross-distro/2017-April/000865.html
As you can see, mostly crickets, other than a suggestion to lock it in
via libvirt.
>> location for where distros should place these firmware images, so let's
>> define this path here to try and minimize distro fragmentation.
>
> hmmm... This makes it appear that nothing is decided upon yet.
If you know of someone else you need me to convince, point me at 'em! :)
>> ---
>> src/qemu/qemu_conf.c | 12
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>
> How would anyone know to use this unless qemu.conf was modified to
> describe the possibility?
>
> See commit id '436dcf0b' for when AAVMF was originally added keeping in
> mind that commit id 'f2f1e388' fixed the names/path. This will give a
> better idea of what additional files should be changed.
OK, thanks - I'll take a look.
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index 73c33d6788..c1bd91935b 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -130,6 +130,8 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr
>> def)
>> #define VIR_QEMU_OVMF_SEC_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd"
>> #define VIR_QEMU_AAVMF_LOADER_PATH "/usr/share/AAVMF/AAVMF_CODE.fd"
>> #define VIR_QEMU_AAVMF_NVRAM_PATH "/usr/share/AAVMF/AAVMF_VARS.fd"
>> +#define VIR_QEMU_AAVMF32_LOADER_PATH "/usr/share/AAVMF/AAVMF32_CODE.fd"
>> +#define VIR_QEMU_AAVMF32_NVRAM_PATH "/usr/share/AAVMF/AAVMF32_VARS.fd"
>
> Not that it's in my wheelhouse, but I'm somewhat surprised by the notion
> to have the AAVMF directory have both 64 and 32 bit files... Guess I'm
> surprised it's not AAVMF32/ since it would seem easier to be able to
> have consistent names of "%s_VARS.fd" and "%s_CODE.fd" to go with the
> /usr/share/%s/ path when trying to "build" a name. I've asked the
> QEMU/OVMF maintainer in this space and get his insights as well.
One of my goals here is to avoid further pollution of the /usr/share
directory. I've had complaints from other Debian Developers about the
existing AAVMF and OVMF subdirs, but we've retained this for
compatibility with libvirt upstream.
-dann
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 07:30 AM, Pavel Hrdina wrote:
> On Tue, May 09, 2017 at 11:30:11AM -0400, John Ferlan wrote:
>> Rather than have repetitive code - create/use a couple of helpers:
>>
>> testStoragePoolObjFindActiveByName and testStoragePoolObjFindInactiveByName
>
> I would wrap this line, it's too long for no reason.
>
OK - I made them a list, e.g.:
testStoragePoolObjFindActiveByName
testStoragePoolObjFindInactiveByName
>> This will also allow for the reduction of some cleanup path logic.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/test/test_driver.c | 256
>> +
>> 1 file changed, 86 insertions(+), 170 deletions(-)
>>
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index efa54ff..9918df6 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -4030,6 +4030,46 @@ testStoragePoolObjFindByName(testDriverPtr privconn,
>>
>>
>> static virStoragePoolObjPtr
>> +testStoragePoolObjFindActiveByName(testDriverPtr privconn,
>> + const char *name)
>> +{
>> +virStoragePoolObjPtr obj;
>> +
>> +if (!(obj = testStoragePoolObjFindByName(privconn, name)))
>> +return NULL;
>> +
>> +if (!virStoragePoolObjIsActive(obj)) {
>> +virReportError(VIR_ERR_OPERATION_INVALID,
>> + _("storage pool '%s' is not active"), name);
>> +virStoragePoolObjUnlock(obj);
>> +return NULL;
>> +}
>> +
>> +return obj;
>> +}
>> +
>> +
>> +static virStoragePoolObjPtr
>> +testStoragePoolObjFindInactiveByName(testDriverPtr privconn,
>> + const char *name)
>> +{
>> +virStoragePoolObjPtr obj;
>> +
>> +if (!(obj = testStoragePoolObjFindByName(privconn, name)))
>> +return NULL;
>> +
>> +if (virStoragePoolObjIsActive(obj)) {
>> +virReportError(VIR_ERR_OPERATION_INVALID,
>> + _("storage pool '%s' is already active"), name);
>
> I would remove the "already" for the error message. Since this is only
> test driver I'll leave it up to you. The reason is that for some APIs
> like "Undefine" the error message doesn't make sense.
>
> Reviewed-by: Pavel Hrdina
>
Done.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/12/2017 04:17 AM, Pavel Hrdina wrote:
> On Tue, May 09, 2017 at 11:30:14AM -0400, John Ferlan wrote:
>> A virStoragePoolObjPtr will be an 'obj'.
>>
>> A virStoragePoolPtr will be a 'pool'.
>
> There is no such change in this commit.
>
Simple enough to remove.
>>
>> NB: Also modify the @matchpool to @matchobj.
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virstorageobj.c | 342
>> +++
>> src/conf/virstorageobj.h | 20 +--
>> 2 files changed, 180 insertions(+), 182 deletions(-)
>>
>> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
>> index dd41701..74a9c67 100644
>> --- a/src/conf/virstorageobj.c
>> +++ b/src/conf/virstorageobj.c
>> @@ -70,15 +70,15 @@ virStoragePoolObjListFree(virStoragePoolObjListPtr pools)
>>
>> void
>> virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
>> -virStoragePoolObjPtr pool)
>> +virStoragePoolObjPtr obj)
>
> This is why I'm not a fan of this change, to make the code consistent
> we should also rename "pools" to "objs" which I don't like so probably
> "objList" or something else.
>
> Pavel
>
I understand the objections and I agree @pools could be something
else... Similarly @devs, @nwfilters, @secrets, @interfaces, etc.
The difference between @pools and @pool/@obj is the usage of @pool/@obj
wasn't consistent. At least there's only one @pools - OK, well two one
in _virStorageDriverState and one in _testDriver.
I guess that's the line I didn't cross.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 10:38 AM, Erik Skultety wrote:
> On Mon, Jul 03, 2017 at 05:25:28PM -0400, John Ferlan wrote:
>> Now that we have a bit more control, let's convert our object into
>> a lockable object and let that magic handle the create and lock/unlock.
>>
>> This also involves creating a virNodeDeviceEndAPI in order to handle
>> the object cleaup for API's that use the Add or Find API's in order
>
> s/cleaup/cleanup
>
eagle eyes
>
> [...]
>>
>> cleanup:
>> -virNodeDeviceObjUnlock(obj);
>> +virNodeDeviceObjEndAPI(&obj);
>> if (ret == -1) {
>> --ncaps;
>> while (--ncaps >= 0)
>> @@ -613,8 +613,7 @@ nodeDeviceDestroy(virNodeDevicePtr device)
>> * to be taken, so grab the object def which will have the various
>> * fields used to search (name, parent, parent_wwnn, parent_wwpn,
>> * or parent_fabric_wwn) and drop the object lock. */
>
> ^This commentary got me thinking. There's a deadlock because of how the locks
> are acquired earlier in this function. Patch 14 solves the deadlock in
> exchange
> for a race, so see my comment in patch 14.
>
> [...]
I'll respond there. Not sure where the deadlock would be here, but maybe
the answer there will clear things up...
>> cleanup:
>> -if (obj)
>> -virNodeDeviceObjUnlock(obj);
>> +virNodeDeviceObjEndAPI(&obj);
>> testDriverUnlock(driver);
>> virNodeDeviceDefFree(def);
>> virObjectUnref(dev);
>> @@ -5596,13 +5595,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
>> * taken, so we have to dup the parent's name and drop the lock
>> * before calling it. We don't need the reference to the object
>> * any more once we have the parent's name. */
>
> Not that this patch would be touching it directly, but ^this commentary is
> pretty much useless now, since @parent_name is useless in this function,
> nodeDeviceDestroy doesn't work that way anymore.
>
> Erik
>
Hmmm, I see said the blind man. Looks like commit id '7ad479d0b' should
have removed @parent_name as well.
That should be a separate patch - I can create and add it to the end.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 10:38 AM, Erik Skultety wrote:
> On Mon, Jul 03, 2017 at 05:25:30PM -0400, John Ferlan wrote:
>> Since virnodedeviceobj now has a self-lockable hash table, there's no
>> need to lock the table from the driver for processing. Thus remove the
>> locks from the driver for NodeDeviceObjList mgmt.
>>
>> Signed-off-by: John Ferlan
>
> [..]
>
>> @@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device)
>> return -1;
>> def = virNodeDeviceObjGetDef(obj);
>>
>> -nodeDeviceLock();
>> -
>
> Consider the following scenario handling the same device:
>
What if I told you that's impossible? You cannot "have" a scsi_hostN,
delete a scsi_hostN, and then have a new one created with the same name.
The scsi_hostN's are an ever increasing value of N. Once created and
deleted, the N will not be reused.
> Thread 1 (udev event handler callback) | Thread 2 (nodeDeviceDestroy)
> ===|=
>| # attempt to destroy a device
>| obj = nodeDeviceObjFindByName()
>|
>| # @obj is locked
>| def = virNodeDeviceObjGetDef(obj)
>|
>| virNodeDeviceObjEndAPI(&obj)
>| # @obj is unlocked
> <--
> # change event |
> udevAddOneDevice() |
>|
> obj = virNodeDeviceObjListFindByName |
> # @obj locked|
> new_device = false |
> # @obj unlocked |
>|
> obj = virNodeDeviceObjListAssignDef |
> # @obj locked|
> virNodeDeviceObjEndAPI(&obj) |
> # @obj unlocked and @def changed |
> -->
>| virNodeDeviceObjListGetParentHost()
>|if (def->parent) ===> SIGSEGV
>
> In patch 12/14 this would have been a deadlock because you first locked the
> @obj, then nodedev driver while udevAddOneDevice did in the reverse order. I
> don't know about NPIV so I'm not sure whether there is another way of removing
> a device and updating the parent device tree, might require an updated model,
> but for now, you need to make a deep copy of @def. I can see that the chance
> of
> getting an 'change' event from udev on a vHBA device is low, but I'm trying to
> look at it from a higher perspective, as we want to be able to remove mdevs
> this way, possibly other devices in the future.
I think what happens is code from virNodeDeviceGetWWNs through
virVHBAManageVport gets placed into a function that handles vHBA's on
deletion. Similarly for CreateXML, vHBA specific code ends up in a
helper function. Those helpers would be called based on the type of
object/device we're talking about (vHBA/mdev).
BTW: I recall doing a review suggesting perhaps creating an mdev pool
driver of sorts. Daniel essentially responded that using the node device
driver and augmenting it to fit the needs would be OK. At the time, I
wasn't specifically thinking about this case, but it certainly qualifies
as something of concern where the existing node device code can make
some assumptions about names that the underlying udev code provides.
You may need to add some extra layer of protection if names can be
reused especially because of this event mgmt "problem"... You may also
need to modify that "if (dev)" code to check if the dev is an mdev and
if so, do something different.
Looks like the code was added by commit id '546fa3ef'. Perhaps as a way
to "follow" how other drivers did things.
"Fortunately" the only consumer of CreateXML/Destroy ends up being vHBA
and we know there's rules around the naming. IIRC - you were letting
MDEV also set the name, right? That is, a 'name' on input is essentially
and happily ignored. So what creates that name? And can you be assured
it's going to be unique for the life/run time of the host? If so,
there's no way a CreateXML could reuse a name that AddOneDevice would be
using, right?
Maybe I need to think some more - it's been a long day already
John
>
> The rest of the patch looks okay, but I want to try it first.
>
> Erik
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
> On 06/02/2017 12:25 PM, John Ferlan wrote:
>> If the virNWFilterSaveConfig in virNWFilterObjListLoadConfig, then jumping
>
> s/,/ fails,/
>
>> to the error label would free the @def owned by the object, but the object
>> is still on the list.
>>
>> Fix this by following proper procedure to clear @def and create a new
>> variable @objdef to handle the object's def after successful assignment.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virnwfilterobj.c | 7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>> index 0027d45..3fb6046 100644
>> --- a/src/conf/virnwfilterobj.c
>> +++ b/src/conf/virnwfilterobj.c
>> @@ -485,6 +485,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr
>> nwfilters,
>> {
>> virNWFilterDefPtr def = NULL;
>> virNWFilterObjPtr obj;
>> +virNWFilterDefPtr objdef;
>> char *configFile = NULL;
>>
>> if (!(configFile = virFileBuildPath(configDir, name, ".xml")))
>> @@ -503,10 +504,12 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr
>> nwfilters,
>>
>> if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
>> goto error;
>> +def = NULL;
>> +objdef = obj->def;
>>
>> /* We generated a UUID, make it permanent by saving the config to disk
>> */
>> -if (!def->uuid_specified &&
>> -virNWFilterSaveConfig(configDir, def) < 0)
>> +if (!objdef->uuid_specified &&
>> +virNWFilterSaveConfig(configDir, objdef) < 0)
>> goto error;
>
> This @objdef variable looks redundant to me. Can't we access obj->def
> directly? Esp. since you're introducing a variable just for a two times
> use. Then again, we often use obj->def->... when it comes to domain
> objects, why not here?
>
If obj->def ever gets "consumed" by the virObject code, then obj->def->x
will fail miserably. That was the original design goal. I've since had
to scale back. I guess I could do "obj->def->uuid_specified" as it
doesn't really matter until the day obj->def-> is no longer accessible.
As a preference - I like the extra local variable. In the long run the
compiler will do away with it, but for me it just reads better that way.
The deeper one gets into a->b->c->d[->e...] the more insane it gets.
Forcing one level of indirection is just more readable to me.
John
> ACK if you drop the @objdef variable.
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
> On 06/02/2017 12:25 PM, John Ferlan wrote:
>> After returning from virNWFilterObjListAssignDef the @obj is locked;
>> however, if virNWFilterSaveConfig fails to save the generated UUID
>> the code jumped to error and returns NULL meaning the caller will
>> not call virNWFilterObjUnlock on the object leaving the object
>> locked on list and ripe for a deadlock on any subsequent Find
>> of all objects or object removal.
>>
>> So rather than jumping to error alter the comment prior to the
>> virNWFilterSaveConfig and just provide a VIR_INFO message for anyone
>> that cares, but allow the code to continue and a subsequent LoadConfig
>> to once again attempt the save of a newly generated UUID.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virnwfilterobj.c | 7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>> index 3fb6046..0343c0a 100644
>> --- a/src/conf/virnwfilterobj.c
>> +++ b/src/conf/virnwfilterobj.c
>> @@ -507,10 +507,13 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr
>> nwfilters,
>> def = NULL;
>> objdef = obj->def;
>>
>> -/* We generated a UUID, make it permanent by saving the config to disk
>> */
>> +/* We generated a UUID, atttempt to make it permanent by saving the
>
> s/ttt/tt/
>
>> + * config to disk. If not successful, no need to fail or remove the
>> + * object as a future load would regenerate a UUID and try again,
>> + * but the existing config would still exist and can be used. */
>> if (!objdef->uuid_specified &&
>> virNWFilterSaveConfig(configDir, objdef) < 0)
>> -goto error;
>> +VIR_INFO("failed to save generated UUID for filter '%s'",
>> objdef->name);
>>
>> VIR_FREE(configFile);
>> return obj;
>>
>
> Well, frankly I'd say that we should not even try to save the config in
> the first place. Load() should really just load. We shouldn't try to
> "fix" XML configs at load time rather then when saving it in define phase.
>
IIRC: this one's a bit weird since if the UUID doesn't exist we "can"
generate one. If we generate one, then we should save it. However,
failing to save shouldn't be called an error because having a UUID isn't
required it's just something we try to "enforce" at some point in time
of the nwfilter. I kind of didn't want to "adjust" that logic. That's a
different patch ;->
John
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
> On 06/02/2017 12:25 PM, John Ferlan wrote:
>> When creating an object, save the configFile name in the object rather
>> than needing to build it up each time for the SaveConfig. This involves
>> adding a @configDir parameter to virNWFilterObjListAssignDef and removing
>> the @configDir from virNWFilterObjSaveConfig.
>
> Why? This keeps the path in memory for the whole life time of the object
> even though it's needed just occasionally - at the beginning and
> probably at the end.
>
OK - again this is something that was originally from the broader scheme
where a ConfigName could be saved in a more opaque object. We do it for
secrets and storage, so this was a bit of monkey-see, monkey-do.
John
(I'm done for the day - I'll pick up looking at the rest of the review
comments tomorrow).
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virnwfilterobj.c | 38 --
>> src/conf/virnwfilterobj.h | 6 +++---
>> src/nwfilter/nwfilter_driver.c | 5 +++--
>> 3 files changed, 26 insertions(+), 23 deletions(-)
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
ping
On 11.04.2017 10:39, Nikolay Shirokovskiy wrote:
> diff from v2:
>
>
> 1. Fix style issues.
> 2. Rework patch for fetching job info
>(save logic to use temporary variable when drop vm lock)
> 3. Update disk stats when block jobs are canceled.
> 4. Adress a few more corner cases.
>
> This patch series add disks stats to domain job info(stats) as
> well as to migration completed event in case nbd scheme is used.
>
> There is little nuisance with qcow2 disks (which is the main scenario
> I guess) tied to the way qemu reports stats for this type of disks.
> For example if we have 64G disk filled only to 1G then stats
> start from 63G and will grow up to 64G on completion. The same way disk stats
> will be reported by this patch.
>
> I guess the better way to express the situation is to say we have 64G 'total',
> and have 'processed' field grow from 0G to 1G, like in case of memory
> stats. [1] is the example of completed memory stats of empty guest
> domain, which show difference between processed and total.
>
> There can be a workaround like getting initial blockjob offset position
> as a zero but is is rather ugly and racy and like uses undocumented
> behaviour.
>
> [1] memory migration stats example
> Memory processed: 3.307 MiB
> Memory remaining: 0.000 B
> Memory total: 1.032 GiB
>
> The above is applied to qemu 2.6 at least.
>
> Patches that were explicitly ACKed in previous review
> (up to style issues) marked with A.
>
> Nikolay Shirokovskiy (16):
> qemu: drop code for VIR_DOMAIN_JOB_BOUNDED and timeRemaining
> A qemu: introduce qemu domain job status
> A qemu: introduce QEMU_DOMAIN_JOB_STATUS_POSTCOPY
> A qemu: drop QEMU_MIGRATION_COMPLETED_UPDATE_STATS
> A qemu: drop excessive zero-out in qemuMigrationFetchJobStatus
> qemu: refactor fetching migration stats
> A qemu: simplify getting completed job stats
> qemu: fail querying destination migration statistics always
> qemu: start all async job with job status active
> qemu: introduce migrating job status
> qemu: always get job condition on getting job stats
> qemu: migrate: show disks stats on job info requests
> qemu: support getting disks stats during stopping block jobs
> qemu: migation: resolve race on getting job info and stopping block jobs
> qemu: migrate: copy disks stats to completed job
> qemu: migration: don't expose incomplete job as complete
>
> src/qemu/qemu_blockjob.c | 1 +
> src/qemu/qemu_domain.c | 38 +--
> src/qemu/qemu_domain.h | 16 ++-
> src/qemu/qemu_driver.c | 95
> src/qemu/qemu_migration.c| 236
> ++-
> src/qemu/qemu_migration.h| 15 ++-
> src/qemu/qemu_migration_cookie.c | 7 +-
> src/qemu/qemu_monitor.c | 5 +-
> src/qemu/qemu_monitor.h | 4 +-
> src/qemu/qemu_monitor_json.c | 4 +-
> src/qemu/qemu_process.c | 10 +-
> tests/qemumonitorjsontest.c | 1 +
> 12 files changed, 273 insertions(+), 159 deletions(-)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
