On 11/13/2017 10:44 PM, Jiri Denemark wrote:
> Some compiler may get confused and decide we are calling strcmp with
> NULL argument from test_virCapsDomainDataLookupLXC. Although this does
> not really happen since the call is guarded with
> (data->machinetype != expect_machinetype), using STRNEQ_NULLABLE is
> easier to understand, less fragile, and doing so makes sure strcmp is
> never called with NULL argument.
>
> Signed-off-by: Jiri Denemark
> ---
> tests/vircapstest.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
We need to call it anyway, so the else branch is redundant here.
Signed-off-by: Erik Skultety
---
It was either this or revert the order of the conditions so that the else
branch/block is actually the bigger one, complying with our guidelines.
src/locking/lock_daemon.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 0d5e999ef..6751b57bc 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -1325,10 +1325,11 @@ int main(int argc, char **argv) {
ret = VIR_LOCK_DAEMON_ERR_NETWORK;
goto cleanup;
}
-} else if (rv == 1) {
-srv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd");
+virObjectUnref(srv);
}
+srv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd");
+
if (timeout != -1) {
VIR_DEBUG("Registering shutdown timeout %d", timeout);
virNetDaemonAutoShutdown(lockDaemon->dmn,
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
So far we were configuring the sound output based on what graphic device
was configured in domain XML. This had a several disadvantages, it's
not transparent, in case of multiple graphic devices it was overwritten
by the last one and there was no simple way how to configure this per
domain.
The new element for devices allows you to configure
which output will be used for each domain, however QEMU has a limitation
that all sound devices will always use the same output because it is
configured by environment variable QEMU_AUDIO_DRV per domain.
For backward compatibility we need to preserve the defaults if no output
is specified:
- for vnc graphic it's by default NONE unless "vnc_allow_host_audio"
was enabled, in that case we use DEFAULT which means it will pass
the environment variable visible by libvirtd
- for sdl graphic by default we pass the environment variable
- for spice graphic we configure the SPICE output
- if no graphic is configured we use by default NONE unless
"nogfx_allow_host_audio" was enabled, in that case we pass
the environment variable
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375433
Signed-off-by: Pavel Hrdina
---
docs/formatdomain.html.in | 4 ++-
src/qemu/qemu_command.c | 84 +--
src/qemu/qemu_domain.c| 54 ++
src/qemu/qemu_process.c | 41 +++
4 files changed, 135 insertions(+), 48 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 3b7c367fc7..ae0d8b86be 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7064,7 +7064,9 @@ qemu-kvm -net nic,model=? /dev/null
the audio output is connected within host. There is mandatory
type attribute where valid values are 'none' to
disable the audio output, 'spice', 'pa', 'sdl', 'alsa', 'oss'.
- This might not be supported by all hypervisors.
+ This might not be supported by all hypervisors. QEMU driver
+ has a limitation that all sound devices have to use the same
+ output.
Watchdog device
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c5c7bd7e54..5cbd1d0d46 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4442,67 +4442,57 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound,
}
-static void
+static int
qemuBuildSoundAudioEnv(virCommandPtr cmd,
- const virDomainDef *def,
- virQEMUDriverConfigPtr cfg)
+ const virDomainDef *def)
{
+char *envStr = NULL;
+
if (def->nsounds == 0) {
virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
-return;
+return 0;
}
-if (def->ngraphics == 0) {
-if (cfg->nogfxAllowHostAudio)
-virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
-else
-virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
-} else {
-switch (def->graphics[def->ngraphics - 1]->type) {
-case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
-/* If using SDL for video, then we should just let it
- * use QEMU's host audio drivers, possibly SDL too
- * User can set these two before starting libvirtd
- */
-virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
+/* QEMU doesn't allow setting different audio output per sound device
+ * so it will always be the same for all devices. */
+switch (def->sounds[0]->output) {
+case VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT:
+/* The default output is used only as backward compatible way to
+ * pass-through environment variables configured before starting
+ * libvirtd. */
+virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
+if (def->ngraphics > 0 &&
+def->graphics[def->ngraphics - 1]->type ==
VIR_DOMAIN_GRAPHICS_TYPE_SDL) {
virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL);
+}
+break;
-break;
-
-case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
-/* Unless user requested it, set the audio backend to none, to
- * prevent it opening the host OS audio devices, since that causes
- * security issues and might not work when using VNC.
- */
-if (cfg->vncAllowHostAudio)
-virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
-else
-virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
-
-break;
-
-case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
-/* SPICE includes native support for tunnelling audio, so we
- * set the audio backend to point at SPICE's own driver
- */
-virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=spice");
-
-break;
-
-case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
-case VIR_DOMAIN_GRAPHICS_TYPE_D
Setting the default audio output depends on specific graphic device
but requires having sound device configured as well and it's the sound
device that handles the audio.
Signed-off-by: Pavel Hrdina
---
src/qemu/qemu_command.c| 84 +++---
.../qemuxml2argv-clock-france.args | 2 +-
2 files changed, 58 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index eb72db33ba..e1ef1b05fa 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4442,10 +4442,62 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound,
}
+static void
+qemuBuildSoundAudioEnv(virCommandPtr cmd,
+ const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg)
+{
+if (def->ngraphics == 0) {
+if (cfg->nogfxAllowHostAudio)
+virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
+else
+virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
+} else {
+switch (def->graphics[def->ngraphics - 1]->type) {
+case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
+/* If using SDL for video, then we should just let it
+ * use QEMU's host audio drivers, possibly SDL too
+ * User can set these two before starting libvirtd
+ */
+virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
+virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL);
+
+break;
+
+case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
+/* Unless user requested it, set the audio backend to none, to
+ * prevent it opening the host OS audio devices, since that causes
+ * security issues and might not work when using VNC.
+ */
+if (cfg->vncAllowHostAudio)
+virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
+else
+virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
+
+break;
+
+case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
+/* SPICE includes native support for tunnelling audio, so we
+ * set the audio backend to point at SPICE's own driver
+ */
+virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=spice");
+
+break;
+
+case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
+case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
+case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
+break;
+}
+}
+}
+
+
static int
qemuBuildSoundCommandLine(virCommandPtr cmd,
const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ virQEMUDriverConfigPtr cfg)
{
size_t i, j;
@@ -4498,6 +4550,9 @@ qemuBuildSoundCommandLine(virCommandPtr cmd,
}
}
}
+
+qemuBuildSoundAudioEnv(cmd, def, cfg);
+
return 0;
}
@@ -7951,15 +8006,6 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr
cfg,
if (graphics->data.vnc.keymap)
virCommandAddArgList(cmd, "-k", graphics->data.vnc.keymap, NULL);
-/* Unless user requested it, set the audio backend to none, to
- * prevent it opening the host OS audio devices, since that causes
- * security issues and might not work when using VNC.
- */
-if (cfg->vncAllowHostAudio)
-virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
-else
-virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
-
return 0;
error:
@@ -8201,10 +8247,6 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr
cfg,
if (graphics->data.spice.keymap)
virCommandAddArgList(cmd, "-k",
graphics->data.spice.keymap, NULL);
-/* SPICE includes native support for tunnelling audio, so we
- * set the audio backend to point at SPICE's own driver
- */
-virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=spice");
return 0;
@@ -8235,13 +8277,6 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
if (graphics->data.sdl.fullscreen)
virCommandAddArg(cmd, "-full-screen");
-/* If using SDL for video, then we should just let it
- * use QEMU's host audio drivers, possibly SDL too
- * User can set these two before starting libvirtd
- */
-virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
-virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL);
-
/* New QEMU has this flag to let us explicitly ask for
* SDL graphics. This is better than relying on the
* default, since the default changes :-( */
@@ -9995,11 +10030,6 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
} else {
virCommandAddArg(cmd, "-nographic");
}
-
-if (cfg->nogfxAllowHostAudio)
-virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
-else
-
On Fri, Nov 10, 2017 at 05:41:51PM -0500, John Ferlan wrote:
>
>
> On 11/10/2017 10:08 AM, Erik Skultety wrote:
> > On Tue, Nov 07, 2017 at 09:39:54PM -0500, John Ferlan wrote:
> >> Whether the @srv/@srvAdm is added to the dmn->servers list or not,
> >> the reference kept for the allocation can be dropped leaving just the
> >> reference for being on the dmn->servers list be the sole deciding
> >> factor when to really free the associated memory. The @dmn dispose
> >> function (virNetDaemonDispose) will handle the Unref of the objects
> >> during the virHashFree.
> >>
> >> Signed-off-by: John Ferlan
> >> ---
> >> daemon/libvirtd.c | 15 +++
> >> 1 file changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> >> index 73f24915df..5c47e49d48 100644
> >> --- a/daemon/libvirtd.c
> >> +++ b/daemon/libvirtd.c
> >> @@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
> >> char *remote_config_file = NULL;
> >> int statuswrite = -1;
> >> int ret = 1;
> >> +int rc;
> >> int pid_file_fd = -1;
> >> char *pid_file = NULL;
> >> char *sock_file = NULL;
> >> @@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
> >> goto cleanup;
> >> }
> >>
> >> -if (virNetDaemonAddServer(dmn, srv) < 0) {
> >> +/* Add @srv to @dmn servers hash table and Unref to allow removal from
> >> + * hash table at @dmn disposal to decide when to free @srv */
> >> +rc = virNetDaemonAddServer(dmn, srv);
> >
> >
> > Sorry for the delay, I've been playing with LXC for 2 days, trying to either
> > debug or just use valgrind on lxc_controller to get the info I need
> > (explanation below), but after those 2 days, I just give up, not worth any
> > more
> > time, if someone knows how I can actually hit the cleanup section in
> > lxc_controller within gdb telling me that it suddenly temporarily disabled
> > breakpoints at the moment it's supposed to stop at one that it had let me
> > to set
> > earlier, I'm all ears. Btw to my biggest surprise valgrind still doesn't
> > handle
> > setns syscall which has probably been the reason why we forbade running LXC
> > under valgrind in 2009, unbelievable.
>
> I feel your pain ;-) Chasing these memory things is challenging
> especially when variable names change or things just aren't consistent.
Well, it wouldn't have to be if one didn't have to fight the mentioned tools so
much - if you've been using fedora since v25 (I don't know whether debian/ubuntu
suffer the same way), you might have come across this weird thing that even if
you compile libvirt with debugging symbols, you're not able to actually step
into non-static functions with "step" (you'd have to use stepi multiple times)
because the linker doesn't generate the PLT stubs for some reason, which is
quite frustrating for someone who debugs a lot and more breakpoints just don't
help here really since some of them might get hit constantly from places I don't
want and disabling them simply doesn't feel efficient like using 'step'.
...
> Anyway, I agree with you upon visual inspection - there's a leak since
> @srv is never Unref'd. Once it's added to the Server hash table, then
> there should be a virObjectUnref(srv).
\o/ This is just one in a ... lot ..., since the Unrefs are almost impossible
to track visually... :)
>
> As for other consumers The reason that log_daemon doesn't do the
> virObjectUnref is that it saves @srv and eventually will Unref it later.
> Unlike the lock_daemon code which uses virNetDaemonGetServer to find the
> @srv; whereas, log_daemon goes direct to logDaemon->srv.
Yeah, that just doesn't feel right, virtlogd is just a copy of virtlockd, I
haven't found a compelling reason why they're different in this aspect, so I
sent simple patches to address that.
>
>
> >
> > Now the actual review.
> > virNetDaemonAddServer is only used when spawning a new daemon or setting up
> > LXC
> > controller. The function essentially does:
> >
> > lock(@dmn)
> > hash_table_add(@srv)
> > ref(@srv)
> > unlock(@dmn)
> >
> > and then you unref @dmn right upon the completion of adding the server to
> > the
> > hash table, what's the purpose of the extra ref when you discard it
> > immediately?
>
> The REF is for the Hash Table. We've added something to the hash table -
> we should increment the refcnt - nothing more nothing less. The UNREF
> for that occurs as part of virHashFree because virHashCreate uses
> virObjectFreeHashData.
Sure, I understand that the ref is for the hash table, but there's no other
caller that would interfere with the object (the idea behind ref counting), you
already have a ref for the daemon, which is not going to need it anymore (we
know this), so you can just pass it onto the hash table, because right now, it
looks odd, you wait dor successful addition to the table (it's not event the
table doing the ref increment), increment ref, return, drop ref - every time I
look at it, it rai
On Sun, Nov 12, 2017 at 09:46:48AM -0500, John Ferlan wrote:
>
> [...]
>
> > Now the actual review.
> > virNetDaemonAddServer is only used when spawning a new daemon or setting up
> > LXC
> > controller. The function essentially does:
> >
> > lock(@dmn)
> > hash_table_add(@srv)
> > ref(@srv)
> > unlock(@dmn)
> >
> > and then you unref @dmn right upon the completion of adding the server to
> > the
> > hash table, what's the purpose of the extra ref when you discard it
> > immediately? Unless I'm failing to see something, I'd prefer to just drop
> > the
> > extra ref in virNetDaemonAddServer and leave the proposed hunks unchanged -
> > you
> > have 1 ref which you got by creating the object, now it should be just
> > simply
> > inserted into the hash table, the additional ref after insertion doesn't
> > make
> > sense, if someone managed to unref it before inserting it into the hash
> > table,
> > hash insert would most likely fail, if not, hashFree surely would, not
> > mentioning that at that point there's no entity that is touching servers.
> >
>
> Since I was "digging" on a different issue - check out how this is done
> elsewhere... Start with virNetServerDispatchNewClient. It'll call the
> ClientNew function which generates a REF. Then it will call AddClient
> which generates a REF. Then because it's on that list and this code is
> theoretically done with it - it will UNREF the client before returning.
Sure, but we shouldn't treat everything in a uniform manner - the fact that we
can do that and it works still doesn't necessarily mean it's right. BTW I only
looked at NetClient briefly, but that too looks to me like the reffing is
unnecessary.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 13, 2017 at 01:36:58PM -0500, John Ferlan wrote:
On 11/13/2017 03:50 AM, Martin Kletzander wrote:
We can't output better memory sizes if we want to be compatible with libvirt
older than the one which introduced /memory/unit, but for new things we can just
output nicer capacity to the user if available. And this function enables that.
Signed-off-by: Martin Kletzander
---
src/libvirt_private.syms | 1 +
src/util/virutil.c | 50
src/util/virutil.h | 3 +++
3 files changed, 54 insertions(+)
Since we have virFormatIntDecimal, why not change the name to be
virFormatPrettySize. I'd also support moving it closer to the virFormat*
function (both .c and .h), but it's not a "requirement"...
Sure, I can do that.
Reviewed-by: John Ferlan
John
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
There's no point in checking if numa->mem_nodes[node].ndistances
is set if we check for numa->mem_nodes[node].distances. However,
it makes sense to check if the sibling node caller passed falls
within boundaries.
Signed-off-by: Michal Privoznik
---
src/conf/numa_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index 7bba4120b..5f0b3f9ed 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1154,7 +1154,7 @@ virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
*/
if (!distances ||
!distances[cellid].value ||
-!numa->mem_nodes[node].ndistances)
+node >= numa->nmem_nodes)
return (node == cellid) ? LOCAL_DISTANCE : REMOTE_DISTANCE;
return distances[cellid].value;
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Signed-off-by: Michal Privoznik
---
docs/news.xml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/docs/news.xml b/docs/news.xml
index 3966710ee..502679917 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -43,7 +43,7 @@
A NUMA hardware architecture supports the notion of distances
between NUMA cells. This can now be specified using the
element within the NUMA cell
- configuration.
+ configuration. Drivers which support this include Xen and QEMU.
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 13, 2017 at 02:22:20PM -0500, John Ferlan wrote:
$subj:
s/virBitampString/virBitmapString/
hehe, good catch
On 11/13/2017 03:50 AM, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_capabilities.c | 4 ++--
src/util/virbitmap.c | 6 --
src/util/virbitmap.h | 2 +-
3 files changed, 7 insertions(+), 5 deletions(-)
FWIW: For some reason 4/21 hasn't yet made it to the mail server near
me. I don't know why, it's just lost or on vacation. The only comment I
have on it is that the commit message "util: Rename virBitmapString
tovirBitmapToString" needs a slight adjustment "...String to virBit..."
You mean add a space between "to" and virBitmapToString? It is there.
I have one adjustment below regarding one arg per line in function...
Consider this a:
Reviewed-by: John Ferlan
For this and 04/21 because I'm way to lazy to figure out how to generate
a reply to something I don't have the original email...
Sure, mentioning here is enough.
John
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 7cb091056b48..c87feefb3be3 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1509,7 +1509,7 @@ int virQEMUCapsParseHelpStr(const char *qemu,
qemuCaps, check_yajl) < 0)
goto cleanup;
-strflags = virBitmapString(qemuCaps->flags);
+strflags = virBitmapString(qemuCaps->flags, true);
VIR_DEBUG("Version %u.%u.%u, cooked version %u, flags %s",
major, minor, micro, *version, NULLSTR(strflags));
VIR_FREE(strflags);
@@ -2376,7 +2376,7 @@ virQEMUCapsClear(virQEMUCapsPtr qemuCaps,
char *virQEMUCapsFlagsString(virQEMUCapsPtr qemuCaps)
{
-return virBitmapString(qemuCaps->flags);
+return virBitmapString(qemuCaps->flags, true);
}
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 57a8fbf82c78..cb6600074781 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -312,17 +312,19 @@ int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool
*result)
/**
* virBitmapString:
* @bitmap: Pointer to bitmap
+ * @prefix: Whether to prepend "0x"
Or "prehex" ;-)
*
* Convert @bitmap to printable string.
*
* Returns pointer to the string or NULL on error.
*/
-char *virBitmapString(virBitmapPtr bitmap)
+char *virBitmapString(virBitmapPtr bitmap, bool prefix)
One line for each argument is the more "recent" style...
Sure, but a) mainly because otherwise it's a long line and b) it's along the
other functions that do the same so the reading style is the same, but here it
would just stand out as the only function, I think. Anyway, I fixed that as
well.
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
size_t sz;
-virBufferAddLit(&buf, "0x");
+if (prefix)
+virBufferAddLit(&buf, "0x");
sz = bitmap->map_len;
diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index ffa3c42d792b..dc8fb71a07b8 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -80,7 +80,7 @@ bool virBitmapIsBitSet(virBitmapPtr bitmap, size_t b)
int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
-char *virBitmapString(virBitmapPtr bitmap)
+char *virBitmapString(virBitmapPtr bitmap, bool prefix)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
char *virBitmapFormat(virBitmapPtr bitmap);
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 13.11.2017 11:35, Martin Kletzander wrote:
> On Thu, Sep 28, 2017 at 09:35:45AM +0300, Nikolay Shirokovskiy wrote:
>> Directories /var/{lib,cache}/libvirt/qemu/ are created by libvirtd on
>> start and their owner:group is changed according to the config. Thus
>> no need to include them in libvirt-daemon-driver-qemu package. Otherwise
>> we see noisy "directory changed" on rpm -V for the package.
>> ---
>> libvirt.spec.in | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index a3bd77f..e20f65c 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -1911,8 +1911,6 @@ exit 0
>> %config(noreplace) %{_sysconfdir}/libvirt/qemu-lockd.conf
>> %config(noreplace) %{_sysconfdir}/logrotate.d/libvirtd.qemu
>> %ghost %dir %attr(0700, root, root) %{_localstatedir}/run/libvirt/qemu/
>> -%dir %attr(0751, %{qemu_user}, %{qemu_group})
>> %{_localstatedir}/lib/libvirt/qemu/
>> -%dir %attr(0750, %{qemu_user}, %{qemu_group})
>> %{_localstatedir}/cache/libvirt/qemu/
>> %{_datadir}/augeas/lenses/libvirtd_qemu.aug
>> %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
>> %{_libdir}/%{name}/connection-driver/libvirt_driver_qemu.so
>
> I agree, however I think this should be done on all subdirectories of
> %{_localstatedir}/{cache,lib}/libvirt/. That directories are owned by
> libvirt-daemon and libvirt-libs respectively, so it should be OK.
I don't think we can generalize this patch for other hypervisors. For example
lxc don't create "%{_localstatedir}/lib/libvirt/lxc/" in runtime thus this
directory need to be installed.
I also give this patch more thought and think we can fix issue differently.
- use %ghost directive just like for run dir so that we cleanup on uninstall
additionally
or
- install dir as root:root and don't change owner at runtime. Why do we need
to make lib and cache group:owner qemu:qemu in the first place? Looks like
we only store qemu caps cache and domain screenshots in cache and both files
are created by daemon itself. And lib dir only have directories which
in turn created by daemon too:
[localhost]# ls dir
boot dnsmasq filesystems images libxl lockd lxc network qemu sanlock
uml xen
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Nov 14, 2017 at 15:47:39 +0100, Michal Privoznik wrote:
> Since we already have such support for libxl all we need is qemu
> driver adjustment. And a test case.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_command.c| 36 +++-
> .../qemuxml2argv-numatune-distances.args | 63 +
> .../qemuxml2argv-numatune-distances.xml| 65
> ++
> tests/qemuxml2argvtest.c | 2 +
> 4 files changed, 165 insertions(+), 1 deletion(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.xml
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index eb72db33b..8b9daaea3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7675,7 +7675,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
> virCommandPtr cmd,
> qemuDomainObjPrivatePtr priv)
> {
> -size_t i;
> +size_t i, j;
> virQEMUCapsPtr qemuCaps = priv->qemuCaps;
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> char *cpumask = NULL, *tmpmask = NULL, *next = NULL;
> @@ -7685,6 +7685,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
> int ret = -1;
> size_t ncells = virDomainNumaGetNodeCount(def->numa);
> const long system_page_size = virGetSystemPageSizeKB();
> +bool numa_distances = false;
>
> if (virDomainNumatuneHasPerNodeBinding(def->numa) &&
> !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
> @@ -7793,6 +7794,39 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>
> virCommandAddArgBuffer(cmd, &buf);
> }
> +
> +/* If NUMA node distance is specified for at least one pair
> + * of nodes, we have to specify all the distances. Even
> + * though they might be the default ones. */
> +for (i = 0; i < ncells; i++) {
> +for (j = 0; j < ncells; j++) {
> +if (!virDomainNumaNodeDistanceSpecified(def->numa, i, j))
> +continue;
> +
> +numa_distances = true;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_DIST)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("setting NUMA distances is not "
> + "supported with this qemu"));
> +goto cleanup;
This capability does not need to be checked in the loop. Also the loop
does not make sense to be finished once you deduct that 'numa_distances'
is true.
> +}
> +}
> +}
> +
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/2017 03:34 AM, Chenjia (C) wrote:
> Dear libvirt expert:
> Thanks for your reply.
> May be our description is not suitable, in last message,'snapshot'
> means the domain state file which 'virsh save ' generate.
>
> Our project detailed steps are as follows:
> 1) create 40 xen guestOS by 'vrish create xen*.xml'
> 2) save 40 guest OS to domain state file by 'virsh save' cmd
> 3) Start multiple processes, each process cycle to do the following job:
> a)virsh restore the domain state file
'virsh restore' has --bypass-cache option which basically enables
O_DIRECT. That can save you couple of seconds.
> b)do same job in guest OS in 20 seconds
> c)virsh destroy the guest OS
>
> We need higher performance on the project , so our question is these as
> last message:
> 1) each our xen state file is almost same except IP address(Win7
> OS with 1G memory), does there have some way to reduce the 40 state file
> space?
I don't think so. I mean, I don't know about Xen that much, but for
instance in QEMU, the saved file contains memory for the guest (among
with the internal state of hypervisor). In general, no guest have the
same content of the memory. Also, you only *think* that the guests are
the same. There is a lot of subtle differences (e.g. internal kernel
state, RNG, network stack, stack randomization, etc.). So having some
shared base image is not really a way to go. But for saving some disk
space you can enable compression for the saved images (possibly not
implemented in Xen driver).
> 2) Do you have some suggestion to improve the performance of
> restore as much as xen state file at same time?
See my replies above. But I'll let Jim reply as he knows more about Xen
than me.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/2017 04:13 PM, Peter Krempa wrote:
> On Tue, Nov 14, 2017 at 15:47:39 +0100, Michal Privoznik wrote:
>> Since we already have such support for libxl all we need is qemu
>> driver adjustment. And a test case.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_command.c| 36 +++-
>> .../qemuxml2argv-numatune-distances.args | 63
>> +
>> .../qemuxml2argv-numatune-distances.xml| 65
>> ++
>> tests/qemuxml2argvtest.c | 2 +
>> 4 files changed, 165 insertions(+), 1 deletion(-)
>> create mode 100644
>> tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args
>> create mode 100644
>> tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.xml
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index eb72db33b..8b9daaea3 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7675,7 +7675,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>> virCommandPtr cmd,
>> qemuDomainObjPrivatePtr priv)
>> {
>> -size_t i;
>> +size_t i, j;
>> virQEMUCapsPtr qemuCaps = priv->qemuCaps;
>> virBuffer buf = VIR_BUFFER_INITIALIZER;
>> char *cpumask = NULL, *tmpmask = NULL, *next = NULL;
>> @@ -7685,6 +7685,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>> int ret = -1;
>> size_t ncells = virDomainNumaGetNodeCount(def->numa);
>> const long system_page_size = virGetSystemPageSizeKB();
>> +bool numa_distances = false;
>>
>> if (virDomainNumatuneHasPerNodeBinding(def->numa) &&
>> !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
>> @@ -7793,6 +7794,39 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>>
>> virCommandAddArgBuffer(cmd, &buf);
>> }
>> +
>> +/* If NUMA node distance is specified for at least one pair
>> + * of nodes, we have to specify all the distances. Even
>> + * though they might be the default ones. */
>> +for (i = 0; i < ncells; i++) {
>> +for (j = 0; j < ncells; j++) {
>> +if (!virDomainNumaNodeDistanceSpecified(def->numa, i, j))
>> +continue;
>> +
>> +numa_distances = true;
>> +
>> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_DIST)) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("setting NUMA distances is not "
>> + "supported with this qemu"));
>> +goto cleanup;
>
> This capability does not need to be checked in the loop. Also the loop
> does not make sense to be finished once you deduct that 'numa_distances'
> is true.
Ah. Correct. Consider this squashed in then:
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index 8b9daaea3..bceafb084 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -7804,17 +7804,18 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
continue;
numa_distances = true;
-
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_DIST)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("setting NUMA distances is not "
- "supported with this qemu"));
-goto cleanup;
-}
+break;
}
}
if (numa_distances) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_DIST)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("setting NUMA distances is not "
+ "supported with this qemu"));
+goto cleanup;
+}
+
for (i = 0; i < ncells; i++) {
for (j = 0; j < ncells; j++) {
size_t distance =
virDomainNumaGetNodeDistance(def->numa, i, j);
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2017-11-13 at 10:36 -0500, John Ferlan wrote:
> > @@ -4776,6 +4777,13 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
> > if (qemuCaps->version >= 2006000)
> > virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT);
> >
> > +/* HPT resizing is supported since QEMU 2.10 on ppc64; unfortunately
> > + * there's no sane way to probe for it */
> > +if (qemuCaps->version >= 201 &&
> > +ARCH_IS_PPC64(qemuCaps->arch)) {
>
> This check differs slightly from qemuDomainDefVerifyFeatures
QEMU capabilities are per-binary, so even though the resize-hpt
property is only supported by the pseries machine type we have to
store it globally. Plus at this point we just have no idea what
machine type the user is eventually going to choose. The capability
name should make it clear that it's pseries only.
> > @@ -7526,6 +7526,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> > }
> > }
> >
> > +if (def->features[VIR_DOMAIN_FEATURE_HPT] ==
> > VIR_TRISTATE_SWITCH_ON) {
> > +const char *str;
> > +
> > +if (!virQEMUCapsGet(qemuCaps,
> > QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("HTP resizing is not supported by this "
> > + "QEMU binary"));
> > +goto cleanup;
> > +}
> > +
> > +str = virDomainHPTResizingTypeToString(def->hpt_resizing);
> > +if (!str) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("Invalid setting for HPT resizing"));
> > +goto cleanup;
> > +}
> > +
> > +virBufferAsprintf(&buf, ",resize-hpt=%s", str);
>
> This one only cares about the capability...
Because by the time we get to build the QEMU command line we will
have made sure the setting is only used along with a compatible
machine type in qemuDomainDefVerifyFeatures() below, no need to
duplicate the check and risk it getting out of sync eventually.
> > @@ -3142,6 +3142,14 @@ qemuDomainDefVerifyFeatures(const virDomainDef *def)
> > return -1;
> > }
> >
> > +if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON &&
> > +!qemuDomainIsPSeries(def)) {
>
> But this one adds the def->machine.os check... I know it's post parse
> so it should thus cause failure before building the command line occurs,
> so the question is should capability use "&& qemuDomainIsPSeries"? Your
> call...
As explained above, it can't :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Table of contents.
I Preface
1. Fleece API
2. Export API
3. Incremental backups
4. Other hypervisors
II Links
I Preface
This is a RFC for external (or pull) backup API in libvirt. There was a series
[1]
with more limited API scope and functionality for this kind of backup API.
Besides other issues the series was abandoned as qemu blockdev-del command has
experimental status at that time. There is also a long pending RFC series for
internal (or push) backup API [2] which however has not much in comman with
this RFC. Also there is RFC with overall agreement to having a backup API in
libvirt [3].
The aim of external backup API is to provide means for 3d party application to
read/write domain disks as block devices for the purpuse of backup. Disk is
read on backup operation and in case of active domain is presented at some
point in time (preferable in some guest consistent state). Disk is written on
restore operation.
As to providing disk state at some point in time one can use existing disks
snapshots for this purpose. However this RFC introduces API to leverage image
fleecing (blockdev-backup command) instead. Image fleecing is somewhat inverse
to snapshots. In case of snapshots writes go to top image thus backing image
stays constant, in case of fleecing writes go to same image as before but old
data is previously popped out to fleece image which have original image as
backing. As a result fleece image became disk snapshot.
Another task of this API is to provide disks for read/write operations. One
could try to leverage libvirt stream API for this purpose but AFAIK clients
want random access to disks data which is not what stream API suitable for.
I'm not sure what is costs of adding block API to libvirt, particularly what it
costs to make it effective implementation at RPC level thus this RFC add means
to export disks data thru existing block interfaces. For qemu it is NBD.
1. Fleece API
So the below API is to provide means to start/stop/query disk image fleecing.
I use BlockSnaphost name for this operation. Other options are Fleecing,
BlockFleecing,
TempBlockSnapshot etc.
/* Start fleecing */
virDomainBlockSnapshotPtr
virDomainBlockSnapshotCreateXML(virDomainPtr domain,
const char *xmlDesc,
unsigned int flags);
/* Stop fleecing */
int
virDomainBlockSnapshotDelete(virDomainBlockSnapshotPtr snapshot,
unsigned int flags);
/* List active fleecings */
virDomainBlockSnapshotList(virDomainPtr domain,
virDomainBlockSnapshotPtr **snaps,
unsigned int flags);
/* Get fleecing description */
char*
virDomainBlockSnapshotGetXMLDesc(virDomainBlockSnapshotPtr snapshot,
unsigned int flags);
/* Get fleecing by name */
virDomainBlockSnapshotPtr
virDomainBlockSnapshotLookupByName(virDomainPtr domain,
const char *name);
Here is a minimal block snapshot xml description to feed creating function:
Below is an example of what getting description function should provide upon
successful block snaphost creation. The difference with the above xml is that
name element (it can be specified on creation as well) and aliases are
generated. Aliases will be useful later to identify block devices on exporting
thru nbd.
5768a388-c1c4-414c-ac4e-eab216ba7c0c
2. Export API
During backup operation we need to provide read access to fleecing image. This
is done thru qemu process nbd server. We just need to specify the disks to
export.
/* start block export */
int
virDomainBlockExportStart(virDomainPtr domain,
const char *xmlDesc,
unsigned int flags);
/* stop block export */
int
virDomainBlockExportStop(virDomainPtr domain,
const char *diskName,
unsigned int flags);
Here is an example of xml for starting function:
qemu nbd server is started upon first disk export start and shutted down upon
last disk export stop. Another option is to control ndb server explicitly. One
way to do it is to consider ndb server a new device so to start/stop/update ndb
server we can use attach/detach/update device functions. Then in block export
start we need to refer to this device somehow. This can be a generated
name/uuid or type/address pair. Actually this approach to expose ndb server
looks more natural to me even it includes more management from client side.
I am not suggesting it in the first place mostly due to hesitations on how to
refer to ndb server on block export.
In any case I'd like to provide export info in active domain config:
This API is used in restore operation too. Domain is started in paused state,
the disks to be restored are exported and backup client fills it with the
backup data.
3.
On Mon, 2017-11-13 at 10:36 -0500, John Ferlan wrote:
> Those against test bloat would point out only one of the three options
> is really necessary... IDRC, but I also see the trend that could be
> started as new things are added that have 10 different options. I'm
> fine with any one of the 3 to be used... Your call though. Since
> 'enabled' is used for the without CAPS test and 'resizing' for the wrong
> machine test, then perhaps 'disabled' should be the one carried through
> - just a thought as all 3 would then "prove" the ability to parse the 3
> options.
Good point.
I've reduced it to just two input files: pseries-hpt-resizing, loaded
once with the capability present and once with the capability absent,
and pseries-hpt-resizing-invalid-machine, which behaves the same as
before.
I'm going to push the series shortly, thanks for your review! :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/13/2017 03:50 AM, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander
> ---
> tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/schemata | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
I have no idea what the bits represent ;-), still...
Reviewed-by: John Ferlan
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/10/2017 04:03 PM, Ján Tomko wrote:
> On Thu, Nov 09, 2017 at 01:43:22PM +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
>>
>> When testing user aliases it was discovered that for 440fx
>> machine type which has default IDE bus builtin, domain cannot
>> start if IDE controller has the user provided alias. This is
>> because for 440fx we don't put the IDE controller onto the
>> command line (since it is builtin) and therefore any device that
>> is plugged onto the bus must use the default alias.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_command.c | 14 +++---
>> tests/qemuxml2argvdata/qemuxml2argv-user-aliases.args | 4 ++--
>> 2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 364196783..6cc77df2e 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1886,9 +1886,17 @@ qemuBuildDriveDevStr(const virDomainDef *def,
>> virBufferAddLit(&opt, "ide-drive");
>> }
>>
>> - if (!(contAlias = virDomainControllerAliasFind(def,
>> VIR_DOMAIN_CONTROLLER_TYPE_IDE,
>> -
>> disk->info.addr.drive.controller)))
>> - goto error;
>> + /* When domain has builtin IDE controller we don't put it
>> onto cmd
>> + * line. Therefore we can't set its alias. In that case, use the
>> + * default one. */
>> + if (qemuDomainHasBuiltinIDE(def)) {
>> + contAlias = "ide";
>
> This logic would fit better inside virDomainControllerAliasFind.
Would it? virDomain*() functions live in src/conf/ and as such should be
driver agnostic. What is builtin controller in one hv can be explicitly
configurable in another (or not exist at all). I'm failing to see reason
for this function to special case drivers. Regardless, it can't call
qemuDomain*(), can it?
>
> Also, the implicit pci-root on i440fx could also use that treatment.
Okay.
>
> We do not format aliases for PS2 mouse and keyboard either, but I cannot
> imagine why the qemu alias would need to match the libvirt alias in that
> case.
Yup.
>
> Any other devices come to mind?
None so far.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 13, 2017 at 03:42:40PM -0500, John Ferlan wrote:
On 11/13/2017 03:50 AM, Martin Kletzander wrote:
Sometimes the size of the bitmap matters and it might not be guessed correctly
when parsing from some type of input. For example virBitmapNewData() has Byte
granularity, virBitmapNewString() has nibble granularity and so on.
virBitmapParseUnlimited() can be tricked into creating huge bitmap that's not
needed (e.g.: "0-2,^"). This function provides a way to shrink the
bitmap. It is not supposed to free any memory.
Is there a specific reason why you don't free memory? Consider that the
corollary virBitmapExpand can always be used to regrow the bitmap. I'm
fine with not free'ing, but maybe someone would want to... OK, sure
they can supply the patch some day, I know...
I don't free it because a) it would cost more time and b) we over-allocate a bit
anyway. Also this is mainly used so that the bitmap size is predictable, not
much else.
Signed-off-by: Martin Kletzander
---
src/libvirt_private.syms | 1 +
src/util/virbitmap.c | 19 +++
src/util/virbitmap.h | 2 ++
3 files changed, 22 insertions(+)
With a couple of notes below handled,
Reviewed-by: John Ferlan
John
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0b78a0681c5e..3986cc523e39 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1369,6 +1369,7 @@ virBitmapParseUnlimited;
virBitmapSetAll;
virBitmapSetBit;
virBitmapSetBitExpand;
+virBitmapShrink;
virBitmapSize;
virBitmapSubtract;
virBitmapToData;
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index ac6ff4f6d26d..95b1f8656907 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -176,6 +176,25 @@ int virBitmapSetBit(virBitmapPtr bitmap, size_t b)
return 0;
}
+
+/**
+ * virBitmapShrink:
+ * @map: Pointer to bitmap
+ * @b: last bit position to be excluded from bitmap
+ *
+ * Resizes the bitmap so that no more than @b bits will fit into it. Nothing
+ * will change if the size is already smaller than @b.
Considering adding, "NB: Does not adjust the map->map_len so that a
subsequent virBitmapExpand doesn't necessarily need to reallocate."
(not required, just a suggestion)
Added
+ */
+void virBitmapShrink(virBitmapPtr map, size_t b)
void
virBitmapStrink(virBitmapPtr map,
size_t b)
done
+{
+if (!map)
+return;
+
+if (map->max_bit >= b)
+map->max_bit = b;
+}
+
+
/**
* virBitmapExpand:
* @map: Pointer to bitmap
diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index 7b2bea8b534c..2464814055de 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -153,4 +153,6 @@ void virBitmapIntersect(virBitmapPtr a, virBitmapPtr b)
void virBitmapSubtract(virBitmapPtr a, virBitmapPtr b)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+void virBitmapShrink(virBitmapPtr map, size_t b);
+
Not that it matters but it's always nice to keep the .h file in the same
relative order as the .c file... So this would move below virBitmapSetBit
I went the other way and moved it in the .c file, I have no idea why I didn't
put it in the end anyway.
#endif
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
https://bugzilla.redhat.com/show_bug.cgi?id=1434451
Just like in 9324f67a572f9b32 we need to put default pci-root
alias onto the command line instead of the one provided by user.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_command.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index eb72db33b..affad43e1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -318,16 +318,25 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
cont->idx == info->addr.pci.bus) {
-contAlias = cont->info.alias;
contIsPHB = virDomainControllerIsPSeriesPHB(cont);
contTargetIndex = cont->opts.pciopts.targetIndex;
-if (!contAlias) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Device alias was not set for PCI "
- "controller with index %u required "
- "for device at address %s"),
- info->addr.pci.bus, devStr);
-goto cleanup;
+
+/* When domain has builtin pci-root controller we don't put it
+ * onto cmd line. Therefore we can't set its alias. In that
+ * case, use the default one. */
+if (!qemuDomainIsPSeries(domainDef) &&
+cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
+contAlias = "pci.0";
+} else {
+contAlias = cont->info.alias;
+if (!contAlias) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Device alias was not set for PCI "
+ "controller with index %u required "
+ "for device at address %s"),
+ info->addr.pci.bus, devStr);
+goto cleanup;
+}
}
break;
}
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Nov 14, 2017 at 07:38:50AM -0500, John Ferlan wrote:
On 11/13/2017 03:50 AM, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander
---
src/conf/capabilities.c| 45 ++
tests/vircaps2xmldata/vircaps-x86_64-caches.xml| 2 +-
.../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml | 4 +-
.../vircaps2xmldata/vircaps-x86_64-resctrl-skx.xml | 4 +-
tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 4 +-
5 files changed, 36 insertions(+), 23 deletions(-)
Reviewed-by: John Ferlan
John
couple of noisy review thoughts below...
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 095ef51c424a..5bf8ac2019f9 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -883,7 +883,8 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
for (i = 0; i < ncaches; i++) {
virCapsHostCacheBankPtr bank = caches[i];
char *cpus_str = virBitmapFormat(bank->cpus);
-bool kilos = !(bank->size % 1024);
+const char *unit = NULL;
+unsigned long long short_size = virPrettySize(bank->size, &unit);
if (!cpus_str)
return -1;
@@ -897,32 +898,44 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
"size='%llu' unit='%s' cpus='%s'",
bank->id, bank->level,
virCacheTypeToString(bank->type),
- bank->size >> (kilos * 10),
- kilos ? "KiB" : "B",
- cpus_str);
+ short_size, unit, cpus_str);
VIR_FREE(cpus_str);
virBufferSetChildIndent(&controlBuf, buf);
for (j = 0; j < bank->ncontrols; j++) {
You could have "virResctrlInfoPtr controls = bank->controls[j];"
done
-bool min_kilos = !(bank->controls[j]->granularity % 1024);
-
-/* Only use KiB if both values are divisible */
-if (bank->controls[j]->min)
-min_kilos = min_kilos && !(bank->controls[j]->min % 1024);
+const char *min_unit;
+unsigned long long gran_short_size =
bank->controls[j]->granularity;
+unsigned long long min_short_size = bank->controls[j]->min;
+
+gran_short_size = virPrettySize(gran_short_size, &unit);
+min_short_size = virPrettySize(min_short_size, &min_unit);
+
+/* Only use the smaller unit if they are different */
Or "if (STRNEQ(unit, min_unit))" - to be more faithful to the comment! I
read this as - if min_short_size is there, then we check the unit by
knowing the math to get the value.
To some degree if the pretty format function allowed one to "choose" a
specific format size that'd perhaps work too, but that's perhaps more
work than it's worth.
if I add STRNEQ there it doesn't allow me to remove any code, it would just add
a line. I need the math after that anyway.
+if (min_short_size) {
+unsigned long long gran_div;
+unsigned long long min_div;
+
+gran_div = bank->controls[j]->granularity / gran_short_size;
+min_div = bank->controls[j]->min / min_short_size;
+
+if (min_div > gran_div) {
+min_short_size *= min_div / gran_div;
+} else if (min_div < gran_div) {
+unit = min_unit;
+gran_short_size *= gran_div / min_div;
+}
+}
virBufferAsprintf(&controlBuf,
"controls[j]->granularity >> (min_kilos *
10));
+ gran_short_size);
-if (bank->controls[j]->min) {
-virBufferAsprintf(&controlBuf,
- " min='%llu'",
- bank->controls[j]->min >> (min_kilos * 10));
-}
+if (min_short_size)
+virBufferAsprintf(&controlBuf, " min='%llu'", min_short_size);
virBufferAsprintf(&controlBuf,
" unit='%s' type='%s' maxAllocs='%u'/>\n",
- min_kilos ? "KiB" : "B",
+ unit,
virCacheTypeToString(bank->controls[j]->scope),
bank->controls[j]->max_allocation);
}
[...]
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hold tight, this is a long one...
It is hard for me to believe it, but the libvirt project is now 12 years old
(born on Nov 2, 2005), and I've been working on it since March 2006, making it
easily the most significant project I've worked on. It started off life as an
attempt to provide a stable application development API for the Xen hypervisor,
interfacing across XenD, XenStore and Xen hypercalls. It was initially just a
plain C library and Python binding, but when we added QEMU support in Feb 2007
the libvirtd daemon was born. That cemented a split of hypervisor drivers into
two distinct architectures, stateless drivers where all logic was in the library
(VMware ESX, VirtualBox, original Xen) and stateful drivers where all logic was
in the daemon (QEMU, LXC, UML, modern Xen).
The project has been wildly successful beyond our expectations, in particular
the hypervisor abstraction layer made it possible for RHEL to switch from using
Xen to KVM while keeping the userspace tooling the same for users. Libvirt is
now used, to some degree, by likely 100's of applications with KVM being the
dominant hypervisor choice by a long way. There is an old adage in the computer
industry though
"Adapt or die"
This is usually applied to companies who see their primary product suddenly
become a commodity, or disappear into irrelevance as new technology disrupts
the market, killing their revenue stream. It is, however, just as reasonable to
apply this to open source projects which can see their core usage scenarios
disrupted by new startup projects & technologies. While the open source code
will never go away, the companies who pay for the project's developers can
quickly reassign them elsewhere, seriously harming the viability of the
community thereafter.
IOW, while Libvirt has seen 12 years of great success, we must not be so naive
to assume we are going to see another 12 years without being disrupted.
Over time we've done alot of work refactoring libvirt code to introduce new
concepts and support new hypervisor targets, but I think its fair to say that at
a high level the architecture is unchanged since we first introduced libvirtd,
and then its multithreaded internals in the 2006-2008 timeframe. We've taken a
fairly conservative, evolutionary approach to our changes. This is good, because
providing stability to our users is a critically important reason for libvirt to
exist. This is bad, because we've not been willing to take risks in short term
that could potentially be very beneficial in the long term (5-10 year time).
I think that now is the time to consider some major architectural changes in the
approach we take. There's no single reason, rather a combination of factors all
coming together to form a compelling case for ambitious change.
Before going further though, I want to highlight one important point:
I am NOT suggesting changing the public API or the XML format in a backwards
incompatible manner. API & XML stability is the single most important part of
libvirt and MUST be maintained on a par with that's available today. IOW we can
add new features, but can't remove what's there already. This even leaves the
door open for providing a libvirt2.so, provided we're willing to still maintain
libvirt.so indefinitely alongside, though that's not something I'd encourage.
The majority of the hard problems we face are not in the API design, or in the
XML format, so that's not a significant limiting factor IMHO.
There are three core areas of libvirt I see that are problematic, and where
the fixes have major implications.
At a very high level what I'm going to suggest is
- Expose key hypervisor specific concepts as fully supported features to
applications. In particular provide a way for an application to launch
QEMU processes directly in their process execution environment, rather
than as a child of libvirtd.
- Explode the libvirtd daemon into a swarm of independant daemons. This
would provide a more reliable system where a single bug doesn't take
out the entire libvirt management daemon. It would allow for better
security isolation of components. It would let session libvirtd
use system daemons for networking & hostdev setup
- Adopt use of Go and gradually convert (all|most) our C code into Go.
This would improve the reliablity of libvirt, by giving us a memory
safe language with garbage collection. It would improve productivity
by letting us spend more time writing interesting code, rather than
wasting time on platform portability or building basic abstractions
for things like OO programming, hash tables, etc (much of the stuff
we have in src/util), no more XML parsers needed (just annotated
struct fields). It would increase the talent pool of potential
contributors to libvirt by lowering the bar to getting work done.
To avoid this mail getting too long, I'll cover each area in a separate mail.
Regards,
Daniel
--
|: https://berrange.com -o-htt
The problem(s)
==
While a hypervisor agnostic API is useful for some users, it is completely
irrelevant, and potentally even painful, for other users. We made some
concessions to this when we introduced hypervisor specific XML namespaces
and option for hypervisor specific add-on APIs. We tell apps these are all
unsupported for production usage though. IOW, have this pony, but you can
never play with it.
The hypervisor agnostic API approach inevitably took us in a direction where
libvirt (or something below it) is in charge of managing the QEMU process
lifecycle. We can't expose the concept of process management upto the client
application because many hypervisors don't present virtual machine as UNIX
processes, or merely have processes as a secondary concept eg with Xen a QEMU
process is just subservient to the main Xen guest domain concept. Essentially
libvirt expects the application to treat the hypervisor / compute host as a
black box and just rely on libvirt APIs for managing the virtual machines,
because that is the only way to provide a hypervisor agnostic view of a
commpute host. This approach also gives parity of functionality regardless of
whether the management app is on a remote machine, vs colocated locally with
libvirtd. Most of the large scale management applications have ended up with a
model where they have a component on each compute host talking to libvirt
locally
over a UNIX socket, with TCP based access only really used for live migration.
Thus the management apps have rarely considered the Linux OS to truely be a
black
box when dealing with KVM. To some degree they all peer inside the box, and wish
to take advantage of some of the concepts Linux exposes to integrate with the
hypervisor.
The inability to directly associate a client with the lifecycle of a single
QEMU process has long been a source of frustration to libguestfs. The level
of indirection forced by use of libvirtd does not map well to how libguestfs
wants to use QEMU. Essentially libguestfs isn't trying to use QEMU in a
system mangement scenario, but rather utilize it as an embedded technology
component. As a result, libguestfs still has its own non-libvirt based way
of spawning QEMU which is often used in preference to its libvirt based impl.
Other apps like libvirt-sandbox have faced the same.
When systemd came into existance, finally providing good mechanisms for
process management in Linux machines, we found a tension between what libvirt
wants todo and what systemd wants todo. The best we've managed is
a compromise where libvirt spawns the guest, but then registers it with
systemd. Users can't directly spawn QEMU guests with systemd and then manage
them with libvirt. We've not seen people seriously try to manage QEMU guests
directly with systemd, but it is fair to say that the combination of systemd
and docker have taken away most potential users of libvirt's LXC driver, as apps
managing containers don't want to treat the host as a blackbox, they want to
have more direct control. The failure to get adoption of the LXC driver serves
as a cautionary tale for what could happen to use of the libvirt QEMU driver in
future.
More recently the increasing interest in use of containers is raising new
interesting architectures for the management of processes. In particular the
Kubernetes project can be considerd to provide cluster-wide management of
processes, aka k8s is systemd for data centers. Again there is interest in
using Kubernetes to manage QEMU guests across the data center. The KubeVirt
project is attempting to bridge the conflicting world views of libvirt and
Kubernetes to build a KVM management system to eventually replace both oVirt
and OpenStack. The need to have libvirtd spawn the QEMU processes is causing
severe complications for KubeVirt architecture, causing them to seriously
consider not using libvirt for management KVM. This issue is a major blocking
item for KubeVirt to the extent that they may well have to abandon use of
libvirt to get the process startup & resource integration model they need.
On Linux as far as hypervisor technology is concerned, KVM has won the
battles and the war. OpenStack user surveys have constantly put KVM/QEMU
on top with at least one order of magnitude higher usage than any other
technology. Amazon was always the major reference for usage of Xen in
public cloud and even they appear to be about to pivot to KVM.
IOW, while providing a hypervisor agnostic management API is still a core
competancy of libvirt, we need to embrace the reality that KVM is the
defacto standard on Linux and better enable people to take avantage of its
unique features, because that is where most of our userbase is.
A second example of limitations of the purely hypervisor agnostic approach
to libvirt is the way our API design is fully synchronous. An application
calling a libvirt API blocks until its execution is complete. This approach
was originally driven by the need to integrate dire
The problem(s)
==
The libvirtd architecture has evolved over time, initially as an expediant
solution to the problem of managing virtual networks and QEMU processes, and
over time came to control all the other resources too. It is only avoided in
the case of the stateless hypervisor drivers which talk to remote RPC systems
(VMWare ESX, HyperV, etc). We later introduced the concepts of loadable
modules, and separate daemons for locking and logging because of the key
requirement that the latter services be re-exec()able while VMs are running.
Despite the existance virtlogd & virtlockd, the libvirtd daemon is clearly
using the monolithic service model. This has a direction impact on both the
reliability and security of libvirtd.
QEMU has the nice characteristic that since it is just a regular process, if
one QEMU goes bad, the other QEMUs continue to operate normally. Libvirtd
then throws away this advantage, by introducing an architecture where if one
QEMU goes bad, it can easily impact all other QEMU processes. This can either
be due to libvirtd crashing, preventing mgmt of all resources, or due to a
rogue QEMU giving libvirtd so much work todo that other jobs get starved.
When we first hit this we introduced multithreading inside libvirtd, which did
help, but made life more complicated. We then saw bottlenecks on the QEMU driver
level locks and had to switch to a lockless driver, with just the VM locks. We
then also had to introduce the job concept, and then async job concept to allow
APIs to complete while the monitor is being used. There are still concurrency
problems in this area, for example QMP event processing in the main thread can
block other API calls and keepalives for arbitrary amounts of time. It is worse
though, because a problem in other areas of libvirtd related to storage,
networking, node devices, and so on can also impact ability to manage QEMU and
vica-verca. This is inherant in the monolithic design of libvirtd where a single
daemon does everything. There are 100's of 1000's of lines of complex code, and
a single bug can impact everything inside libvirtd.
The monolithic model is bad for security too. Given the broad set of features
supported by libvirtd it is impossible to write any meaningful SELinux policy
to lock down its capabilities, unless you're willing to simply block large
feature sets. What is worse is that many of these features require root
privileges, so libvirtd has a whole needs to run as root, and has no security
confinement. Libvirtd meanwhile has to directly interact with non-trusted
components such as the QEMU monitor console, so its security is paramount to
preventing a malicious QEMU from escaping its confinement. To the best of my
knowledge no one has tried to break out of QEMU by attacking libvirtd via QMP,
but that's probably just because they've not told us.
The final problem with libvirtd is the split between system and session mode.
We've long told people that session mode is for desktop virt and system mode
is for server virt, but this simple explanation of roles fails in the real
world. It has been a source of pain for libguestfs for example, which wants
to be able to simply run QEMU with the same rights as the application which
invokes libguestfs. The system vs session distinction means it often hits
problems where the app using libguestfs can read the disk file, but QEMU
launched by libvirtd on libguestfs' behalf cannot read it.
Then there is the fact that with session mode, network connectivity is a
disaster. We hacked around this by using a setuid helper, which lets the admin
grant a user the ability to access a specific bridge device. The mgmt app though
is locked out of all the virtual network management APIs with the session
instance. The conceptual model here is really wrong. Just because you want to
have the QEMU processes runing under the unprivileged user, doesn't imply that
you want the network managements APIs under the same user account. In
retrospect,
simply duplicating the privileged libvirtd functionality in a non-privileged
libvirtd was a clear mistake. Some areas of functionality inherantly require
a privilegd environment and should only ever have run inside root libvirtd.
The solution(s)
===
As noted above, we made some baby-steps towards a modular daemon architecture
when we introduced virtlockd and virlogd. It is now time to fully commit to a
modular design and explode libvirtd into a swarm of daemons each responsible for
a clearly demarked task. Such a decomposition would naturally fall across the
internal driver boundaries, giving a virtnwfilterd, virtnetworkd, virtstoraged,
virtnodedevd, etc. We have to maintain compatibility with our existing client
API implementation though. This libvirtd would have to still accept connections
from the client and route the RPC request directly onto the modular daemon. We
could also enhance the client API to directly know how to connect to the modular
daemons, bypass
The Problem(s)
==
When libvirt was created, C was the only viable choice for anything aiming to be
a core system library component. At that time 2005, aside from C there were
common choices of Java, Python, Perl. Java was way too heavy for a low level
system component, Python was becoming popular but not widely used for low level
system services and Perl was on a downward trend. None of them are accessible to
arbitrary languages as libraries, without providing a RPC based API service. As
it turns out libvirt did end up having RPC based approach for many virt drivers,
but the original approach was to be a pure library component.
IOW it is understandable why C was chosen back in 2005, but 12 years on the
world
around us has changed significantly. It has long been accepted that C is a very
challenging language to write "safe" applications. By "safe" I mean avoiding the
many problems that lead to critical security bugs. In particular the lack of a
safe memory management framework leads to memory leaks, double free's, stack or
heap corruption and more. The lack of strict type safety just compounds these
problems. We've got many tools to help us in this area, and at times have tried
to design our APIs to avoid problems, but there's no getting away from fact that
even the best programmers will continually screw up memory management leading to
crashes & security flaws. It is just a fact of life when using C, particularly
if
you want to be fast at accepting new feature proposals.
It is no surprise that there have been no new mainstream programming languages
in
years (decades) which provide an inherantly unsafe memory management framework.
Even back in 2005 security was a serious challenge, but in the last 10+ years
the situation has only got worse with countless high profile security bugs a
direct result of the choice to use C. Given the threat's faced today, one has to
seriously consider the wisdom of writing any new system software in C. In
another
10 years time, it would not surprise me if any system software still using C is
considered an obsolete relic, and ripe for a rewrite in a memory safe language.
There are long term implications for the potential pool of contributors in the
future. There has always been a limited pool of programmers able todo a good job
in C, compared to those who know higher level languages like Python/Java. A
programmer write bad code in any language, but in C/C++ that bad code quickly
turns into a serious problem. Libvirt has done ok despite this, but I feel our
level of contribution, particularly "drive by" patch submissions, is held back
by use of C. Move forward another 10 years, and while C will certainly exist, I
struggle to imagine the talent pool being larger. On the contrary I would expect
it to shrink, certainly in relative terms, and possibly in absolute terms, as
other new languages take C's place for low level systems programming. 10 years
ago, Docker would have been written in C, but they took the sensible decision to
pick Go instead. This is happening everywhere I look, and if not Go, then Rust.
We push up against the boundaries of what's sane todo in C in other ways too.
For portability across operating systems, we have to rely on GNULIB to try
to sanitize the platform inconsistencies where we use POSIX, and assume that
any 3rd party libraries we use have done likewise.
Even then, we've tried to avoid using the platform APIs because their designs
are often too unsafe to risk using directly (strcat, malloc, free), or are not
thread safe (APIs lacking _r variants). So we build our own custom C platform
library on top of the base POSIX system, re-inventing the same wheel that every
other project written in C invents. Every time we have to do work at the core C
platform level, it is diverting time away from doing working managing higher
level concepts.
Our code is following an object oriented design in many areas, but such a notion
is foreign to C, so we have to bolt a poor-mans OO framework on the side. This
feeds back into the memory safety problem, because our OO invention cannot be
type checked reliably at compile time, making it easy to do unsafe things with
objects. It relies on reference counting because there's no automatic memory
management.
The other big trend of the past 10 years has been the increase in CPU core
counts. My first libvirt dev machine had 1 physical CPU with no cores or threads
or NUMA. My current libvirt dev machine has 2 CPUs, each with 6 cores, for 12
logical CPUs. Common server machines have 32/64 logical CPUs, and high end has
100's of CPUs. In 10 years, we'll see high end machines with 1000's of CPUs and
entry level with mere 100's. IOW good concurrency is going to be key for any
scalable application. Libvirt is actually doing reasonably well in this respect
via our heavily threaded libvirtd daemon. It is not without cost though with
ever more complex threading & locking models, which still have scalability
problems. P
On Tue, Oct 24, 2017 at 07:34 PM +0200, Prerna Saxena
wrote:
> As noted in
> https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html
> libvirt-QEMU driver handles all async events from the main loop.
> Each event handling needs the per-VM lock to make forward progress. In
> the case where an async event is received for the same VM which has an
> RPC running, the main loop is held up contending for the same lock.
What's about the remaining qemuMonitorCallbacks? The main event loop can
still be 'blocked' by e.g. qemuProcessHandleMonitorEOF if the VM is
already locked by a worker thread. In fact, we currently have a problem
with D-Bus which causes a D-Bus call (of a worker thread) to run into
the timeout of 30 seconds. During these 30 seconds the main event loop
is stuck.
I tried the patch series and got a segmentation fault:
Thread 1 "libvirtd" received signal SIGSEGV, Segmentation fault.
0x03ff98faa452 in virEnqueueVMEvent (qlist=0x3ff908ce760,
ev=) at ../../src/qemu/qemu_event.c:153 153
vmq_entry->ev->ev_id = vmq->last->ev->ev_id + 1;
(gdb) bt
#0 0x03ff98faa452 in virEnqueueVMEvent (qlist=0x3ff908ce760,
ev=) at ../../src/qemu/qemu_event.c:153
#1 0x03ff98fc3564 in qemuProcessEnqueueEvent (mon=,
vm=, ev=, opaque=0x3ff90548ec0) at
../../src/qemu/qemu_process.c:1864
#2 0x03ff98fe4804 in qemuMonitorEnqueueEvent
(mon=mon@entry=0x3ff4c007440, ev=0x2aa1e0104c0) at
../../src/qemu/qemu_monitor.c:1325
#3 0x03ff98fe7102 in qemuMonitorEmitShutdown
(mon=mon@entry=0x3ff4c007440, guest=,
seconds=seconds@entry=1510683878, micros=micros@entry=703956) at
../../src/qemu/qemu_monitor.c:1365
#4 0x03ff98ffc19a in qemuMonitorJSONHandleShutdown
(mon=0x3ff4c007440, data=, seconds=1510683878,
micros=) at ../../src/qemu/qemu_monitor_json.c:552
#5 0x03ff98ffbb8a in qemuMonitorJSONIOProcessEvent
(mon=mon@entry=0x3ff4c007440, obj=obj@entry=0x2aa1e012030) at
../../src/qemu/qemu_monitor_json.c:208
#6 0x03ff99002138 in qemuMonitorJSONIOProcessLine
(mon=mon@entry=0x3ff4c007440, line=0x2aa1e010460 "{\"timestamp\":
{\"seconds\": 1510683878, \"microseconds\": 703956}, \"event\":
\"SHUTDOWN\"}", msg=msg@entry=0x0) at
../../src/qemu/qemu_monitor_json.c:237
#7 0x03ff990022b4 in qemuMonitorJSONIOProcess
(mon=mon@entry=0x3ff4c007440, data=0x2aa1e014bc0 "{\"timestamp\":
{\"seconds\": 1510683878, \"microseconds\": 703956}, \"event\":
\"SHUTDOWN\"}\r\n", len=85, msg=msg@entry=0x0) at
../../src/qemu/qemu_monitor_json.c:279
#8 0x03ff98fe4b44 in qemuMonitorIOProcess
(mon=mon@entry=0x3ff4c007440) at ../../src/qemu/qemu_monitor.c:443
#9 0x03ff98fe5d00 in qemuMonitorIO (watch=,
fd=, events=0, opaque=0x3ff4c007440) at
../../src/qemu/qemu_monitor.c:697
#10 0x03ffa68d6442 in virEventPollDispatchHandles (nfds=, fds=0x2aa1e013990) at ../../src/util/vireventpoll.c:508
#11 0x03ffa68d66c8 in virEventPollRunOnce () at
../../src/util/vireventpoll.c:657
#12 0x03ffa68d44e4 in virEventRunDefaultImpl () at
../../src/util/virevent.c:327
#13 0x03ffa6a83c5e in virNetDaemonRun (dmn=0x2aa1dfe3eb0) at
../../src/rpc/virnetdaemon.c:838
#14 0x02aa1df29cc4 in main (argc=, argv=) at ../../daemon/libvirtd.c:1494
>
> This impacts scalability, and should be addressed on priority.
>
> Note that libvirt does have a 2-step deferred handling for a few event
> categories, but (1) That is insufficient since blockign happens before
> the handler could disambiguate which one needs to be posted to this
> other queue.
> (2) There needs to be homogeniety.
>
> The current series builds a framework for recording and handling VM
> events.
> It initializes per-VM event queue, and a global event queue pointing to
> events from all the VMs. Event handling is staggered in 2 stages:
> - When an event is received, it is enqueued in the per-VM queue as well
> as the global queues.
> - The global queue is built into the QEMU Driver as a threadpool
> (currently with a single thread).
> - Enqueuing of a new event triggers the global event worker thread, which
> then attempts to take a lock for this event's VM.
> - If the lock is available, the event worker runs the function handling
> this event type. Once done, it dequeues this event from the global
> as well as per-VM queues.
> - If the lock is unavailable(ie taken by RPC thread), the event worker
> thread leaves this as-is and picks up the next event.
> - Once the RPC thread completes, it looks for events pertaining to the
> VM in the per-VM event queue. It then processes the events serially
> (holding the VM lock) until there are no more events remaining for
> this VM. At this point, the per-VM lock is relinquished.
>
> Patch Series status:
> Strictly RFC only. No compilation issues. I have not had a chance to
> (stress) test it after rebase to latest master.
> Note that documentation and test coverage is TBD, since a few open
> points remain.
>
> Known issues/ caveats:
> - RPC handling time will become non-dete
On Sun, 2017-11-05 at 15:29 +, intrigeri+libv...@boum.org wrote:
> From: intrigeri
>
> This set of rules was proposed by Christian Boltz >
> on https://bugzilla.opensuse.org/show_bug.cgi?id=1065123.
> ---
> examples/apparmor/usr.sbin.libvirtd | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/examples/apparmor/usr.sbin.libvirtd
> b/examples/apparmor/usr.sbin.libvirtd
> index d2831aa491..8d61d154e1 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -30,7 +30,20 @@
># Needed for vfio
>capability sys_resource,
>
> - mount,
> + mount options=(rw,rslave) -> /,
> + mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/,
> +
> + mount options=(rw, move) /dev/ ->
> /{var/,}run/libvirt/qemu/*.dev/,
> + mount options=(rw, move) /dev/hugepages/ ->
> /{var/,}run/libvirt/qemu/*.hugepages/,
> + mount options=(rw, move) /dev/mqueue/->
> /{var/,}run/libvirt/qemu/*.mqueue/,
> + mount options=(rw, move) /dev/pts/ ->
> /{var/,}run/libvirt/qemu/*.pts/,
> + mount options=(rw, move) /dev/shm/ ->
> /{var/,}run/libvirt/qemu/*.shm/,
> +
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ ->
> /dev/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.hugepages/ ->
> /dev/hugepages/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.mqueue/->
> /dev/mqueue/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.pts/ ->
> /dev/pts/,
> + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.shm/ ->
> /dev/shm/,
>
These all look fine. I suspect the rules may need to be fine-tuned for
different distributions, but that's ok, this is example policy and we
can iterate/adjust as needed.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/13/2017 03:50 AM, Martin Kletzander wrote:
> With this commit we finally have a way to read and manipulate basic resctrl
> settings. Locking is done only on exposed functions that read/write from/to
> resctrlfs. Not in fuctions that are exposed in virresctrlpriv.h as those are
functions
> only supposed to be used from tests.
>
> Signed-off-by: Martin Kletzander
> ---
> src/Makefile.am |2 +-
> src/libvirt_private.syms | 12 +
> src/util/virresctrl.c | 1012
> -
> src/util/virresctrl.h | 59 +++
> src/util/virresctrlpriv.h | 32 ++
> 5 files changed, 1115 insertions(+), 2 deletions(-)
> create mode 100644 src/util/virresctrlpriv.h
>
This is a *lot* of code! I wasn't able to run through Coverity mainly
because I have some stuff in a local branch that conflicts with earlier
patches. If you push those, then I can apply these later patches and let
Coverity have a peek on memory leaks or other strangeness that I could
have missed below. I'll reserve the right to come back here again ;-) I
think there's only a few "missed things".
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 1d24231249de..ad113262fbb0 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -167,7 +167,7 @@ UTIL_SOURCES = \
> util/virprocess.c util/virprocess.h \
> util/virqemu.c util/virqemu.h \
> util/virrandom.h util/virrandom.c \
> - util/virresctrl.h util/virresctrl.c \
> + util/virresctrl.h util/virresctrl.c util/virresctrlpriv.h
> \
> util/virrotatingfile.h util/virrotatingfile.c \
> util/virscsi.c util/virscsi.h \
> util/virscsihost.c util/virscsihost.h \
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b24728ce4a1d..37bac41e618b 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2532,6 +2532,18 @@ virRandomInt;
> # util/virresctrl.h
> virCacheTypeFromString;
> virCacheTypeToString;
> +virResctrlAllocAddPID;
> +virResctrlAllocCreate;
> +virResctrlAllocForeachSize;
> +virResctrlAllocFormat;
> +virResctrlAllocGetFree;
> +virResctrlAllocMasksAssign;
> +virResctrlAllocNewFromInfo;
> +virResctrlAllocRemove;
> +virResctrlAllocSetID;
> +virResctrlAllocSetSize;
> +virResctrlAllocUpdateMask;
> +virResctrlAllocUpdateSize;
> virResctrlGetInfo;
> virResctrlInfoGetCache;
>
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 6c6692e78f42..ac1b38436bb2 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -23,7 +23,7 @@
> #include
> #include
>
> -#include "virresctrl.h"
> +#include "virresctrlpriv.h"
>
> #include "c-ctype.h"
> #include "count-one-bits.h"
> @@ -151,6 +151,153 @@ virResctrlInfoNew(void)
> }
>
>
> +/* Alloc-related definitions and AllocClass-related functions */
> +typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
> +typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
> +struct _virResctrlAllocPerType {
> +/* There could be bool saying whether this is set or not, but since
> everything
> + * in virResctrlAlloc (and most of libvirt) goes with pointer arrays we
> would
> + * have to have one more level of allocation anyway, so this stays
> faithful to
> + * the concept */
> +unsigned long long **sizes;
> +size_t nsizes;
> +
> +/* Mask for each cache */
> +virBitmapPtr *masks;
> +size_t nmasks;
> +};
> +
> +typedef struct _virResctrlAllocPerLevel virResctrlAllocPerLevel;
> +typedef virResctrlAllocPerLevel *virResctrlAllocPerLevelPtr;
> +struct _virResctrlAllocPerLevel {
> +virResctrlAllocPerTypePtr *types; /* Indexed with enum virCacheType */
> +};
> +
> +struct _virResctrlAlloc {
> +virObject parent;
> +
> +virResctrlAllocPerLevelPtr *levels;
> +size_t nlevels;
> +
> +char *id; /* The identifier (any unique string for now) */
> +char *path;
> +};
> +
> +static virClassPtr virResctrlAllocClass;
> +
> +static void
> +virResctrlAllocDispose(void *obj)
> +{
> +size_t i = 0;
> +size_t j = 0;
> +size_t k = 0;
> +
> +virResctrlAllocPtr resctrl = obj;
> +
> +for (i = 0; i < resctrl->nlevels; i++) {
> +virResctrlAllocPerLevelPtr level =
> resctrl->levels[--resctrl->nlevels];
Again the odd (to me at least) looking loop control that's reducing the
for loop end condition.
> +
> +if (!level)
> +continue;
> +
> +for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
> +virResctrlAllocPerTypePtr type = level->types[j];
> +
> +if (!type)
> +continue;
> +
> +for (k = 0; k < type->nsizes; k++)
> +VIR_FREE(type->sizes[k]);
> +
> +VIR_FREE(type->sizes);
what about type->masks[k]
You could create a Free function for each entry too.
> +VIR_FREE(type);
> +}
> +VIR_FREE(level->types);
> +VI
On Wed, Nov 15, 2017 at 12:07 AM, Marc Hartmayer <
mhart...@linux.vnet.ibm.com> wrote:
> On Tue, Oct 24, 2017 at 07:34 PM +0200, Prerna Saxena <
> saxenap@gmail.com> wrote:
> > As noted in
> > https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html
> > libvirt-QEMU driver handles all async events from the main loop.
> > Each event handling needs the per-VM lock to make forward progress. In
> > the case where an async event is received for the same VM which has an
> > RPC running, the main loop is held up contending for the same lock.
>
> What's about the remaining qemuMonitorCallbacks? The main event loop can
> still be 'blocked' by e.g. qemuProcessHandleMonitorEOF if the VM is
> already locked by a worker thread. In fact, we currently have a problem
> with D-Bus which causes a D-Bus call (of a worker thread) to run into
> the timeout of 30 seconds. During these 30 seconds the main event loop
> is stuck.
>
>
EOF handling in the current series is still yet another event, and hence is
done only once worker threads complete.
It needs to go into a priority thread pool, and should wake up RPC workers.
> I tried the patch series and got a segmentation fault:
>
> Thread 1 "libvirtd" received signal SIGSEGV, Segmentation fault.
> 0x03ff98faa452 in virEnqueueVMEvent (qlist=0x3ff908ce760,
> ev=) at ../../src/qemu/qemu_event.c:153 153
> vmq_entry->ev->ev_id = vmq->last->ev->ev_id + 1;
> (gdb) bt
> #0 0x03ff98faa452 in virEnqueueVMEvent (qlist=0x3ff908ce760,
> ev=) at ../../src/qemu/qemu_event.c:153
> #1 0x03ff98fc3564 in qemuProcessEnqueueEvent (mon=,
> vm=, ev=, opaque=0x3ff90548ec0) at
> ../../src/qemu/qemu_process.c:1864
> #2 0x03ff98fe4804 in qemuMonitorEnqueueEvent
> (mon=mon@entry=0x3ff4c007440, ev=0x2aa1e0104c0) at
> ../../src/qemu/qemu_monitor.c:1325
> #3 0x03ff98fe7102 in qemuMonitorEmitShutdown
> (mon=mon@entry=0x3ff4c007440, guest=,
> seconds=seconds@entry=1510683878, micros=micros@entry=703956) at
> ../../src/qemu/qemu_monitor.c:1365
> #4 0x03ff98ffc19a in qemuMonitorJSONHandleShutdown
> (mon=0x3ff4c007440, data=, seconds=1510683878,
> micros=) at ../../src/qemu/qemu_monitor_json.c:552
> #5 0x03ff98ffbb8a in qemuMonitorJSONIOProcessEvent
> (mon=mon@entry=0x3ff4c007440, obj=obj@entry=0x2aa1e012030) at
> ../../src/qemu/qemu_monitor_json.c:208
> #6 0x03ff99002138 in qemuMonitorJSONIOProcessLine
> (mon=mon@entry=0x3ff4c007440, line=0x2aa1e010460 "{\"timestamp\":
> {\"seconds\": 1510683878, \"microseconds\": 703956}, \"event\":
> \"SHUTDOWN\"}", msg=msg@entry=0x0) at
> ../../src/qemu/qemu_monitor_json.c:237
> #7 0x03ff990022b4 in qemuMonitorJSONIOProcess
> (mon=mon@entry=0x3ff4c007440, data=0x2aa1e014bc0 "{\"timestamp\":
> {\"seconds\": 1510683878, \"microseconds\": 703956}, \"event\":
> \"SHUTDOWN\"}\r\n", len=85, msg=msg@entry=0x0) at
> ../../src/qemu/qemu_monitor_json.c:279
> #8 0x03ff98fe4b44 in qemuMonitorIOProcess
> (mon=mon@entry=0x3ff4c007440) at ../../src/qemu/qemu_monitor.c:443
> #9 0x03ff98fe5d00 in qemuMonitorIO (watch=,
> fd=, events=0, opaque=0x3ff4c007440) at
> ../../src/qemu/qemu_monitor.c:697
> #10 0x03ffa68d6442 in virEventPollDispatchHandles (nfds= out>, fds=0x2aa1e013990) at ../../src/util/vireventpoll.c:508
> #11 0x03ffa68d66c8 in virEventPollRunOnce () at
> ../../src/util/vireventpoll.c:657
> #12 0x03ffa68d44e4 in virEventRunDefaultImpl () at
> ../../src/util/virevent.c:327
> #13 0x03ffa6a83c5e in virNetDaemonRun (dmn=0x2aa1dfe3eb0) at
> ../../src/rpc/virnetdaemon.c:838
> #14 0x02aa1df29cc4 in main (argc=, argv= out>) at ../../daemon/libvirtd.c:1494
>
>
Thanks for trying it out. Let me look into this.
> >
> > This impacts scalability, and should be addressed on priority.
> >
> > Note that libvirt does have a 2-step deferred handling for a few event
> > categories, but (1) That is insufficient since blockign happens before
> > the handler could disambiguate which one needs to be posted to this
> > other queue.
> > (2) There needs to be homogeniety.
> >
> > The current series builds a framework for recording and handling VM
> > events.
> > It initializes per-VM event queue, and a global event queue pointing to
> > events from all the VMs. Event handling is staggered in 2 stages:
> > - When an event is received, it is enqueued in the per-VM queue as well
> > as the global queues.
> > - The global queue is built into the QEMU Driver as a threadpool
> > (currently with a single thread).
> > - Enqueuing of a new event triggers the global event worker thread, which
> > then attempts to take a lock for this event's VM.
> > - If the lock is available, the event worker runs the function
> handling
> > this event type. Once done, it dequeues this event from the global
> > as well as per-VM queues.
> > - If the lock is unavailable(ie taken by RPC thread), the event
> worker
> > thread leaves this as-is and picks up the next event.
> > - Once the RPC thread c
On Tue, Nov 14, 2017 at 04:20:45PM -0500, John Ferlan wrote:
Would be nice to have more information here regarding what this is
Only after sending have I found out that I somehow missed few commits when
adding commit messages. This is one of the examples, another one is "conf:
cachetune". I can resend part of the series or just add it as a reply to the
mail if you want.
It seems this is just some new test... It works, but does it add to
test bloat? Could it be merged into existing skx test? IOW: does the
existing test "need" to have just one bank? I'm OK with the separate
test, but I guess it's just not clear why there needs to be a separate
one (I could have missed something subtle too). In any case -
For example the commit message that's missing here would say that this
particular test data actually make future test from patch 19 to have more
coverage. There are test cases with comments on why particular cases should
fail and I wanted to cover all obvious failures with only a few system<->domain
combinations. I would have to find the table and the matrix of all of them that
I wrote somewhere on a paper to tell you why exactly this one is needed, but it
is written somewhere, maybe it's just in trash =D
Reviewed-by: John Ferlan
John
[apologies for the "timeliness" of reviews - it's been a distracting
day... Sometimes it's more worthwhile to just turn off IRC ;-)]
No need to apologize, it's very fast review, I thought I will not get any for
couple of weeks =D
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
From: Chen Hanxiao
In the definition of virHookQemuOpType and virHookNetworkOpType,
we should use 'stopped' rather than 'shutdown'.
Signed-off-by: Chen Hanxiao
---
docs/hooks.html.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/docs/hooks.html.in b/docs/hooks.html.in
index 05156c30c..2c4c39b77 100644
--- a/docs/hooks.html.in
+++ b/docs/hooks.html.in
@@ -316,7 +316,7 @@
executes prior to the object (guest or network) being created.
This allows the object start operation to be aborted if the script
returns indicating failure.
- The "shutdown" operation for the guest and network hook scripts,
+ The "stopped" operation for the guest and network hook scripts,
executes after the object (guest or network) has stopped. If
the hook script indicates failure in its return, the shut down of the
object cannot be aborted because it has already been performed.
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
