On Tue, Mar 10, 2020 at 18:23:42 +0100, Ján Tomko wrote:
> On a Tuesday in 2020, Daniel P. Berrangé wrote:
[...]
> > > While ugly I still think that for greppability/searchability it's way
> > > better to keep the object name using camel case. In the end as you've
> > > pointed out it's just for the specific methods named above. Making it
> > > look ugly is IMO less worse than missing something when trying to
> > > understand how the code works.
> >
> > IIRC, Ján raised the issue of avoiding mixed camelcase/snakecase
> > namings originally, so CC'ing to see if he's ok with changing
> > back again ?
>
> I don't care about it enough to send another e-mail after this one :)
>
> However the case convetion is a part of the macro documentation:
> https://developer.gnome.org/gobject/unstable/gobject-Type-Information.html#G-DEFINE-TYPE:CAPS
Hmm, sorry I didn't really look into that in the first place so thanks
for teaching me. While I don't like yet another convention, I'm okay
with the naming in this patch based on the above.
On Tue, Mar 10, 2020 at 17:30:28 +, Daniel Berrange wrote:
> On Tue, Mar 10, 2020 at 06:20:49PM +0100, Ján Tomko wrote:
> > On a Tuesday in 2020, Gaurav Agrawal wrote:
> > > ---
> > > src/qemu/qemu_domain.c | 36
> > > src/qemu/qemu_domain.h | 6 --
> > > src/qemu/qemu_process.c | 4 ++--
> > > 3 files changed, 26 insertions(+), 20 deletions(-)
> > >
> >
> > [...]
> >
> > > @@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr
> > > qemuDomainLogContextNew(virQEMUDriverPtr driver,
> > > return ctxt;
> > >
> > > error:
> > > -virObjectUnref(ctxt);
> > > +if (ctxt)
> > > +g_object_unref(ctxt);
> >
> > g_object_unref is safe to call with a NULL argument, the "if (ctxt)"
> > check is not needed here.
>
> I'm not so sure on that.
>
> g_clear_object API docs explicitly say that it is OK if the object is NULL:
>
>
> https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-clear-object
I'd prefer we agree on using this one globally on the same basis we had
for using VIR_FREE even on cleanup paths.
On a Wednesday in 2020, Peter Krempa wrote:
On Tue, Mar 10, 2020 at 17:30:28 +, Daniel Berrange wrote:
On Tue, Mar 10, 2020 at 06:20:49PM +0100, Ján Tomko wrote:
> On a Tuesday in 2020, Gaurav Agrawal wrote:
> > ---
> > src/qemu/qemu_domain.c | 36
> > src/qemu/qemu_domain.h | 6 --
> > src/qemu/qemu_process.c | 4 ++--
> > 3 files changed, 26 insertions(+), 20 deletions(-)
> >
>
> [...]
>
> > @@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr
qemuDomainLogContextNew(virQEMUDriverPtr driver,
> > return ctxt;
> >
> > error:
> > -virObjectUnref(ctxt);
> > +if (ctxt)
> > +g_object_unref(ctxt);
>
> g_object_unref is safe to call with a NULL argument, the "if (ctxt)"
> check is not needed here.
I'm not so sure on that.
The code itself does check for NULL, it's hidden in the
g_return_if_fail (G_IS_OBJECT (object));
call which boils down to
g_type_check_instance_is_fundamentally_a
Even though the documentation does not mention NULL is safe, it has to be,
since it's
used as a cleanup function in gobject/gobject-autocleanups.h
g_clear_object API docs explicitly say that it is OK if the object is NULL:
https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-clear-object
I'd prefer we agree on using this one globally on the same basis we had
for using VIR_FREE even on cleanup paths.
Sounds good. Most of the unrefs will be handled by g_auto anyway.
Jano
signature.asc
Description: PGP signature
On 3/5/20 3:47 PM, Nikolay Shirokovskiy wrote:
Sync was introduced in [1] to check for ga presence. This
check is racy but in the era before serial events are available
there was not better solution I guess.
Can you elaborate more on the raciness? There should never be more than
one thread talking on the agent monitor.
In case we have the events the sync function is different. It allows us
to flush stateless ga channel from remnants of previous communications.
But we need to do it only once. Until we get timeout on issued command
channel state is ok.
[1] qemu_agent: Issue guest-sync prior to every command
Signed-off-by: Nikolay Shirokovskiy
---
src/qemu/qemu_agent.c| 13 -
src/qemu/qemu_agent.h| 3 ++-
src/qemu/qemu_process.c | 3 ++-
tests/qemumonitortestutils.c | 3 ++-
4 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index cd25ef6cd3..2de53efb7a 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -104,6 +104,8 @@ struct _qemuAgent {
int watch;
bool running;
+bool singleSync;
+bool inSync;
I wonder if we can do this with just inSync and not have singleSync. I
mean, it looks like at all places where singleSync is used we have
access to qemuCaps so it should be as easy as:
qemuAgentOpen();
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE)) {
if *qemuAgentGuestSync(mon) < 0)
goto error;
mon->inSync = true;
and then qemuagentGuestSync() would be a NOP if inSync is set. But it
would also not set it. But maybe I'm pushing it too far, it's just that
I'm confused by the name 'singleSync'. Probably 'hasSingleSync' would be
better? The boolean reflect whether qemu has the VSERPORT_CHANGE
capability and thus libvirt knows when GA connects/disconnects.
'singleSync' just doesn't ring that bell at first.
Michal
On 3/5/20 3:47 PM, Nikolay Shirokovskiy wrote:
needReply added in [1] looks redundant. Indeed it is set to false only
when mon->await_event is set too (the only exception qemuAgentFSTrim
which is mistaken).
However it fixes the issue when qemuAgentCommand exits on error path and
mon->await_event is not reset. Let's instead reset mon->await_event properly.
Also remove "Woken up by event" debug message as it can be misleading.
We can get it also if monitor is closed due to serial changed event
currently. Anyway both qemuAgentClose and qemuAgentNotifyEvent log
itself.
[1] qemu: make sure agent returns error when required data are missing
Signed-off-by: Nikolay Shirokovskiy
---
src/qemu/qemu_agent.c | 47 ---
1 file changed, 22 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 2de53efb7a..605c9f563e 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1112,7 +1112,6 @@ static int
qemuAgentCommand(qemuAgentPtr mon,
virJSONValuePtr cmd,
virJSONValuePtr *reply,
- bool needReply,
int seconds)
{
int ret = -1;
@@ -1121,17 +1120,16 @@ qemuAgentCommand(qemuAgentPtr mon,
int await_event = mon->await_event;
*reply = NULL;
+memset(&msg, 0, sizeof(msg));
if (!mon->running) {
virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
_("Guest agent disappeared while executing command"));
-return -1;
+goto cleanup;
}
if (qemuAgentGuestSync(mon) < 0)
-return -1;
-
-memset(&msg, 0, sizeof(msg));
+goto cleanup;
if (!(cmdstr = virJSONValueToString(cmd, false)))
goto cleanup;
@@ -1149,9 +1147,7 @@ qemuAgentCommand(qemuAgentPtr mon,
/* If we haven't obtained any reply but we wait for an
* event, then don't report this as error */
if (!msg.rxObject) {
-if (await_event && !needReply) {
-VIR_DEBUG("Woken up by event %d", await_event);
-} else {
Please keep this debug printing. It doesn't hurt anything and has
potential of helping us to debug.
+if (!await_event) {
if (mon->running)
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing monitor reply object"));
@@ -1169,6 +1165,7 @@ qemuAgentCommand(qemuAgentPtr mon,
cleanup:
VIR_FREE(cmdstr);
VIR_FREE(msg.txBuffer);
+mon->await_event = QEMU_AGENT_EVENT_NONE;
This is the part I don't like about this patch. The rest looks fine. Why
is this needed? Either the mon->await_event is not set by the caller, or
if set the the event handler will clear it out by calling
qemuAgentNotifyEvent(). Or am I missing something?
return ret;
}
@@ -1269,7 +1266,7 @@ int qemuAgentShutdown(qemuAgentPtr mon,
mon->await_event = QEMU_AGENT_EVENT_RESET;
else
mon->await_event = QEMU_AGENT_EVENT_SHUTDOWN;
-ret = qemuAgentCommand(mon, cmd, &reply, false,
+ret = qemuAgentCommand(mon, cmd, &reply,
VIR_DOMAIN_QEMU_AGENT_COMMAND_SHUTDOWN);
virJSONValueFree(cmd);
@@ -1312,7 +1309,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char
**mountpoints,
if (!cmd)
goto cleanup;
-if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
+if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0)
goto cleanup;
if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
@@ -1349,7 +1346,7 @@ int qemuAgentFSThaw(qemuAgentPtr mon)
if (!cmd)
return -1;
-if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
+if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0)
goto cleanup;
if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
@@ -1386,7 +1383,7 @@ qemuAgentSuspend(qemuAgentPtr mon,
return -1;
mon->await_event = QEMU_AGENT_EVENT_SUSPEND;
-ret = qemuAgentCommand(mon, cmd, &reply, false, mon->timeout);
+ret = qemuAgentCommand(mon, cmd, &reply, mon->timeout);
virJSONValueFree(cmd);
virJSONValueFree(reply);
@@ -1415,7 +1412,7 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon,
if (!(cmd = virJSONValueFromString(cmd_str)))
goto cleanup;
-if ((ret = qemuAgentCommand(mon, cmd, &reply, true, timeout)) < 0)
+if ((ret = qemuAgentCommand(mon, cmd, &reply, timeout)) < 0)
goto cleanup;
if (!(*result = virJSONValueToString(reply, false)))
@@ -1442,7 +1439,7 @@ qemuAgentFSTrim(qemuAgentPtr mon,
if (!cmd)
return ret;
-ret = qemuAgentCommand(mon, cmd, &reply, false, mon->timeout);
+ret = qemuAgentCommand(mon, cmd, &reply, mon->timeout);
Oh this is fun. This 'false' you are removing should have been true as
the comm
On 3/11/20 8:29 AM, Lin Ma wrote:
Verified, This patch fixes the namespace issue while PR in guest with
lun passed-through multipath device node.
Awesome, thanks for testing it! If you want, I can add Tested-by: to the
commit message to give you credit.
Michal
On Tue, Mar 10, 2020 at 07:25:46PM +0100, Andrea Bolognani wrote:
> On Tue, 2020-03-10 at 16:00 +, Daniel P. Berrangé wrote:
> > The split daemon model is intended to allow us to address this
> > long standing design flaw, by allowing the QEMU session driver
> > to optionally talk to a secondary driver running with different
> > privileges, instead of the instance running with the same privs.
> >
> > So currently we have
> >
> >
> >
> >
> >
> > This refers to a secondary driver running at the same privilege
> > level.
> >
> > I expected we'd extend it to allow
> >
> >
> >
> >
> >
> > This explicitly requests the secondary driver running with elevated
> > privileges.
>
> This makes perfect sense to me, and in fact I believe you're
> basically suggesting what I was arguing for earlier :)
>
> In your scenario, when you don't specify a scope you get the same
> one as the primary driver is using (this matches the current
> behavior): so if you are using qemu:///session, every
> will use network:///session unless you explicitly specify
> scope='system', at which point network:///system will be used; in
> the same fashion, if you're connected to qemu:///embed, the default
> for s should be network:///embed, with the possibility
> to use either network:///session (with scope='session') or, most
> likely, network:///system (with scope='system').
No, I'm not talking about using the same URI for the secondary
drivers, I'm talking about using the same *privilege* level.
ie, qemu:///system and a qemu:///embed running as root should
both use the privileged network/storage driver. The qemu:///session
and qemu:///embed running as non-root should both default to
the unprivileged network/storage drivers.
> > With such functionality present, it logically then also extends to
> > cover connections to daemons running in different namespaces.
>
> I'm still unclear on how this scenario, which would apparently have
> multiple eg. privileged virtnetworkd, running at the same time but in
> different network namespaces, would work.
There would need to be some selection method, most likely a way
to explicitly specify the socket path, but this is a longish way
into the future.
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 :|
On Wed, Mar 11, 2020 at 10:26:24AM +0100, Ján Tomko wrote:
> On a Wednesday in 2020, Peter Krempa wrote:
> > On Tue, Mar 10, 2020 at 17:30:28 +, Daniel Berrange wrote:
> > > On Tue, Mar 10, 2020 at 06:20:49PM +0100, Ján Tomko wrote:
> > > > On a Tuesday in 2020, Gaurav Agrawal wrote:
> > > > > ---
> > > > > src/qemu/qemu_domain.c | 36
> > > > > src/qemu/qemu_domain.h | 6 --
> > > > > src/qemu/qemu_process.c | 4 ++--
> > > > > 3 files changed, 26 insertions(+), 20 deletions(-)
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > @@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr
> > > > > qemuDomainLogContextNew(virQEMUDriverPtr driver,
> > > > > return ctxt;
> > > > >
> > > > > error:
> > > > > -virObjectUnref(ctxt);
> > > > > +if (ctxt)
> > > > > +g_object_unref(ctxt);
> > > >
> > > > g_object_unref is safe to call with a NULL argument, the "if (ctxt)"
> > > > check is not needed here.
> > >
> > > I'm not so sure on that.
> > >
>
> The code itself does check for NULL, it's hidden in the
> g_return_if_fail (G_IS_OBJECT (object));
> call which boils down to
> g_type_check_instance_is_fundamentally_a
Yes, I think I can see this now, but I wonder if that's an intentionale
guarantee or not.
> Even though the documentation does not mention NULL is safe, it has to be,
> since it's
> used as a cleanup function in gobject/gobject-autocleanups.h
The G_DEFINE_AUTOPTR_CLEANUP_FUNC macro expands to code which
has an explicit "if(ptr)" check in it, so the actual func
doesn't require this.
> > >
> > > https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-clear-object
> >
> > I'd prefer we agree on using this one globally on the same basis we had
> > for using VIR_FREE even on cleanup paths.
>
> Sounds good. Most of the unrefs will be handled by g_auto anyway.
Yep, makes sense in majority of cases.
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 :|
On 11.03.2020 12:38, Michal Privoznik wrote:
> On 3/5/20 3:47 PM, Nikolay Shirokovskiy wrote:
>> Sync was introduced in [1] to check for ga presence. This
>> check is racy but in the era before serial events are available
>> there was not better solution I guess.
>
> Can you elaborate more on the raciness? There should never be more than one
> thread talking on the agent monitor.
The race is in another dimension) So sync checks for GA presence, it only waits
for 5s so if
there is no GA the actual command with no timeout won't block. But GA can go
down right
after successful sync so command can block. This is true if there are no serial
events
in the latter case agent monitor will be closed when GA go down and command
will unblock.
>
>>
>> In case we have the events the sync function is different. It allows us
>> to flush stateless ga channel from remnants of previous communications.
>> But we need to do it only once. Until we get timeout on issued command
>> channel state is ok.
>>
>> [1] qemu_agent: Issue guest-sync prior to every command
>>
>> Signed-off-by: Nikolay Shirokovskiy
>> ---
>> src/qemu/qemu_agent.c | 13 -
>> src/qemu/qemu_agent.h | 3 ++-
>> src/qemu/qemu_process.c | 3 ++-
>> tests/qemumonitortestutils.c | 3 ++-
>> 4 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>> index cd25ef6cd3..2de53efb7a 100644
>> --- a/src/qemu/qemu_agent.c
>> +++ b/src/qemu/qemu_agent.c
>> @@ -104,6 +104,8 @@ struct _qemuAgent {
>> int watch;
>> bool running;
>> + bool singleSync;
>> + bool inSync;
>>
>
> I wonder if we can do this with just inSync and not have singleSync. I mean,
> it looks like at all places where singleSync is used we have access to
> qemuCaps so it should be as easy as:
>
> qemuAgentOpen();
> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE)) {
> if *qemuAgentGuestSync(mon) < 0)
> goto error;
> mon->inSync = true;
>
>
> and then qemuagentGuestSync() would be a NOP if inSync is set. But it would
> also not set it. But maybe I'm pushing it too far, it's just that I'm
> confused by the name 'singleSync'. Probably 'hasSingleSync' would be better?
> The boolean reflect whether qemu has the VSERPORT_CHANGE capability and thus
> libvirt knows when GA connects/disconnects. 'singleSync' just doesn't ring
> that bell at first.
>
If we try to sync only on monitor opening then we can't sync if for example
command timeouts and
GA stays connect (thus no serial event and monitor won't be closed). With
current patch we just
try to sync when we need to send next command to GA.
I guess singleSync is better. It just reflects that we don't need to sync
before every
command (in order to avoid hangs) in case there is serial events. Strictly
speaking
it is not correct to use sync to check for GA presence because of the race and
as to
using it as a means to flush channel it works well without sereal events as
well.
Nikolay
Le 9 mars 2020 à 14:03, Michal Privoznik a écrit :
>
> On 3/6/20 6:49 PM, Daniel P. Berrangé wrote:
>>> On Fri, Mar 06, 2020 at 06:24:15PM +0100, Andrea Bolognani wrote:
>>> Recently I've been working on integrating qemu:///embed into an
>>> application. It's been reasonably smooth so far :)
>>>
>>> There's one thing, however, that has caused a bit of confusion, and
>>> I would like to clarify whether my expectations are incorrect, there
>>> are genuine bugs in the implementation that need to be addressed, or
>>> maybe the experimental nature of embedded driver support results in
>>> not all scenarios having yet been considered and catered for.
>>>
>>> Using virt-qemu-run as an example, when I run it as root I see that
>>>
>>> * the domain doesn't show up in the output of 'virsh list' for
>>> qemu:///system;
>> This is good.
>>> * it does, however, show up in the output of 'machinectl', with
>>> class=vm and service=libvirt-qemu;
>> This is bad. It is one of the gaps we need to deal with.
>> A long while back I proposed a domain XML option for this:
>> https://www.redhat.com/archives/libvir-list/2017-December/msg00729.html
>>
>> The "none" case meant inherit the cgroups placement of the parent
>> libvirtd (or now the app using the embedded driver), and would be
>> a reasonable default for the embedded case.
>> For the other cases, we certainly need to do something to ensure
>> uniqueness. This is possibly as simple as including a fixed
>> prefix like "qemu-$PID" where $PID is the app embedding the
>> driver. That said, we support closing the app, and re-starting
>> using the same embedded driver directory, which would change
>> PID.
>
> Instead of PID we can derive the value from the root path, e.g. i-node of the
> root dir or some substring of the path hash, say first 6 letters of
> md5($root_path). This would guarantee the stability of the prefix across app
> restarts (if we assume all root dirs to be on one FS so we don't have
> clashing i-node numbers).
>
Root path hash is more robust also in other cases, like file copy/rename often
used for atomic updates.
> Michal
>
Commit <207709a03149e910323be2c8d1305313b30cdeaa> added `root` attribute
to `virStateInitialize` function and marked it as NONNULL which is not
correct because in `daemonRunStateInit` we call it with NULL.
Signed-off-by: Pavel Hrdina
---
src/libvirt_internal.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 00ef7aaf25..72c61274a7 100644
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -33,8 +33,7 @@ int virStateInitialize(bool privileged,
bool mandatory,
const char *root,
virStateInhibitCallback inhibit,
- void *opaque)
-ATTRIBUTE_NONNULL(2);
+ void *opaque);
int virStateCleanup(void);
int virStateReload(void);
int virStateStop(void);
--
2.24.1
Commit <92d412149ce563da48ba299ec49c07d825370cbf> introduced `xmlopt`
attribute to `virDomainDefFormat` function and marked it as NONNULL
which is not correct because in hyperv dirver we call that function
with NULL as `xmlopt`.
Signed-off-by: Pavel Hrdina
---
src/conf/domain_conf.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 91b776c28a..8d6bc5057d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3132,7 +3132,7 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned
int flags);
char *virDomainDefFormat(virDomainDefPtr def,
virDomainXMLOptionPtr xmlopt,
unsigned int flags)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ATTRIBUTE_NONNULL(1);
char *virDomainObjFormat(virDomainObjPtr obj,
virDomainXMLOptionPtr xmlopt,
unsigned int flags)
--
2.24.1
Missing blurb in the cover letter.
On a Wednesday in 2020, Pavel Hrdina wrote:
Pavel Hrdina (3):
domain_conf: fix ATTRIBUTE_NONNULL for virDomainDefFormat
libvirt_internal: fix ATTRIBUTE_NONNULL for virStateInitialize
vircommand: fix ATTRIBUTE_NONNULL for virCommandAddArg*
Do these actually help Coverity do its job or all they're good for
is finding out where we failed to update the attributes?
Jano
src/conf/domain_conf.h | 2 +-
src/libvirt_internal.h | 3 +--
src/util/vircommand.h | 5 ++---
3 files changed, 4 insertions(+), 6 deletions(-)
--
2.24.1
signature.asc
Description: PGP signature
On Wed, Mar 11, 2020 at 01:25:50PM +0100, Ján Tomko wrote:
> Missing blurb in the cover letter.
>
> On a Wednesday in 2020, Pavel Hrdina wrote:
> > Pavel Hrdina (3):
> > domain_conf: fix ATTRIBUTE_NONNULL for virDomainDefFormat
> > libvirt_internal: fix ATTRIBUTE_NONNULL for virStateInitialize
> > vircommand: fix ATTRIBUTE_NONNULL for virCommandAddArg*
> >
>
> Do these actually help Coverity do its job or all they're good for
> is finding out where we failed to update the attributes?
Now that I'm trying to setup coverity scans using scan.coverity.com they
actually help coverity do its job and I managed to run into these issues
while building libvirt using coverity.
Pavel
signature.asc
Description: PGP signature
On 11.03.2020 12:38, Michal Privoznik wrote:
> On 3/5/20 3:47 PM, Nikolay Shirokovskiy wrote:
>> needReply added in [1] looks redundant. Indeed it is set to false only
>> when mon->await_event is set too (the only exception qemuAgentFSTrim
>> which is mistaken).
>>
>> However it fixes the issue when qemuAgentCommand exits on error path and
>> mon->await_event is not reset. Let's instead reset mon->await_event properly.
>>
>> Also remove "Woken up by event" debug message as it can be misleading.
>> We can get it also if monitor is closed due to serial changed event
>> currently. Anyway both qemuAgentClose and qemuAgentNotifyEvent log
>> itself.
>>
>> [1] qemu: make sure agent returns error when required data are missing
>>
>> Signed-off-by: Nikolay Shirokovskiy
>> ---
>> src/qemu/qemu_agent.c | 47 ---
>> 1 file changed, 22 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>> index 2de53efb7a..605c9f563e 100644
>> --- a/src/qemu/qemu_agent.c
>> +++ b/src/qemu/qemu_agent.c
>> @@ -1112,7 +1112,6 @@ static int
>> qemuAgentCommand(qemuAgentPtr mon,
>> virJSONValuePtr cmd,
>> virJSONValuePtr *reply,
>> - bool needReply,
>> int seconds)
>> {
>> int ret = -1;
>> @@ -1121,17 +1120,16 @@ qemuAgentCommand(qemuAgentPtr mon,
>> int await_event = mon->await_event;
>> *reply = NULL;
>> + memset(&msg, 0, sizeof(msg));
>> if (!mon->running) {
>> virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
>> _("Guest agent disappeared while executing
>> command"));
>> - return -1;
>> + goto cleanup;
>> }
>> if (qemuAgentGuestSync(mon) < 0)
>> - return -1;
>> -
>> - memset(&msg, 0, sizeof(msg));
>> + goto cleanup;
>> if (!(cmdstr = virJSONValueToString(cmd, false)))
>> goto cleanup;
>> @@ -1149,9 +1147,7 @@ qemuAgentCommand(qemuAgentPtr mon,
>> /* If we haven't obtained any reply but we wait for an
>> * event, then don't report this as error */
>> if (!msg.rxObject) {
>> - if (await_event && !needReply) {
>> - VIR_DEBUG("Woken up by event %d", await_event);
>> - } else {
>
> Please keep this debug printing. It doesn't hurt anything and has potential
> of helping us to debug.
This debug line is not correct. I reflected it in the commit message.
I actually get this line during debug session when agent monitor is closed
during VM shutdown on serial event and not shutdown event. Serial event
just comes first.
>
>> + if (!await_event) {
>> if (mon->running)
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("Missing monitor reply object"));
>> @@ -1169,6 +1165,7 @@ qemuAgentCommand(qemuAgentPtr mon,
>> cleanup:
>> VIR_FREE(cmdstr);
>> VIR_FREE(msg.txBuffer);
>> + mon->await_event = QEMU_AGENT_EVENT_NONE;
>
> This is the part I don't like about this patch. The rest looks fine. Why is
> this needed? Either the mon->await_event is not set by the caller, or if set
> the the event handler will clear it out by calling qemuAgentNotifyEvent(). Or
> am I missing something?
The whole point of the patch is in this line)
Yeah, you are talking of success paths. But on error paths mon->await_event is
not cleared currently so next command which need reply
can be finish on await event without needReply which lead to SIGSGEV. This is
what patch [1] fixes by introducing needReply which don't need to be cleared
as it is on stack.
I guess scenario is next given the bug [2] mentioned in [1]:
- vm shutdown command is sent to GA
- it is finished with timeout (60 s), mon->await_event is not cleared
- guest ping is sent
- guest shutdowned and ping command get awakend by this event as
mon->await_event is set (bug)
So needReply argument is helpful but it is cleaner just to reset
mon->await_event properly.
[1] qemu: make sure agent returns error when required data are missing
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1058149
Nikolay
>
>> return ret;
>> }
>> @@ -1269,7 +1266,7 @@ int qemuAgentShutdown(qemuAgentPtr mon,
>> mon->await_event = QEMU_AGENT_EVENT_RESET;
>> else
>> mon->await_event = QEMU_AGENT_EVENT_SHUTDOWN;
>> - ret = qemuAgentCommand(mon, cmd, &reply, false,
>> + ret = qemuAgentCommand(mon, cmd, &reply,
>> VIR_DOMAIN_QEMU_AGENT_COMMAND_SHUTDOWN);
>> virJSONValueFree(cmd);
>> @@ -1312,7 +1309,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char
>> **mountpoints,
>> if (!cmd)
>> goto cleanup;
>> - if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
>> + if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0)
>> goto cleanup;
>>
This is now based on top of a different implementation of the fix for
late opening of backing chain during block-copy:
https://www.redhat.com/archives/libvir-list/2020-March/msg00317.html
Changes:
- fixes suggested by Pavel
- changes to capability use
- rebased on master
This series uses blockdev-reopen which was not yet stabilized in qemu,
thus is experimental for now, but since incremental backup is
experimental there's no problem with that.
You can fetch the full branch along with patches for enabling the bits
at:
git fetch https://gitlab.com/pipo.sk/libvirt.git backup-commit
You also need to use patched qemu which you can fetch from:
git fetch https://gitlab.com/pipo.sk/qemu.git reopen-fixes
Peter Krempa (19):
qemu: capabilities: Add QEMU_CAPS_BLOCKDEV_REOPEN
qemu: monitor: Add handler for blockdev-reopen
qemu: block: implement helpers for blockdev-reopen
qemuCheckpointDiscardBitmaps: Reopen images for bitmap modifications
qemuCheckpointDiscardBitmaps: Use correct field for checkpoint bitmap
name
qemuDomainBlockCommit: Move checks depending on capabilities after
liveness check
qemu: domain: Extract formatting of 'commit' blockjob data into a
function
qemu: domain: Extract parsing of 'commit' blockjob data into a
function
qemu: blockjob: Store list of bitmaps disabled prior to commit
qemublocktest: Fix and optimize fake image chain
qemu: block: Implement helpers for dealing with bitmaps during block
commit
qemublocktest: Add tests for handling of bitmaps during block-commit
qemublocktest: Add more tests for block-commit bitmap handling with
snapshots
qemublocktest: Add tests of broken bitmap chain handling during
block-commit
qemuBlockJobDiskNewCommit: Propagate 'disabledBitmapsBase'
qemuDomainBlockCommit: Handle bitmaps on start of commit
qemuDomainBlockPivot: Handle merging of bitmaps when pivoting an
active block-commit
qemu: blockjob: Handle bitmaps after finish of normal block-commit
qemu: blockjob: Re-enable bitmaps after failed block-copy
src/qemu/qemu_block.c | 320 ++
src/qemu/qemu_block.h | 23 ++
src/qemu/qemu_blockjob.c | 97 +-
src/qemu/qemu_blockjob.h | 3 +
src/qemu/qemu_capabilities.c | 1 +
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_checkpoint.c| 9 +-
src/qemu/qemu_domain.c| 110 --
src/qemu/qemu_driver.c| 78 -
src/qemu/qemu_monitor.c | 13 +
src/qemu/qemu_monitor.h | 3 +
src/qemu/qemu_monitor_json.c | 21 ++
src/qemu/qemu_monitor_json.h | 4 +
tests/qemublocktest.c | 126 ++-
.../bitmapblockcommit/basic-1-2 | 119 +++
.../bitmapblockcommit/basic-1-3 | 119 +++
.../bitmapblockcommit/basic-2-3 | 2 +
.../bitmapblockcommit/snapshots-1-2 | 49 +++
.../bitmapblockcommit/snapshots-1-3 | 76 +
.../bitmapblockcommit/snapshots-1-4 | 126 +++
.../bitmapblockcommit/snapshots-1-5 | 130 +++
.../bitmapblockcommit/snapshots-2-3 | 49 +++
.../bitmapblockcommit/snapshots-2-4 | 99 ++
.../bitmapblockcommit/snapshots-2-5 | 103 ++
.../bitmapblockcommit/snapshots-3-4 | 72
.../bitmapblockcommit/snapshots-3-5 | 76 +
.../bitmapblockcommit/snapshots-4-4 | 11 +
.../bitmapblockcommit/snapshots-4-5 | 33 ++
.../snapshots-synthetic-broken-1-2| 27 ++
.../snapshots-synthetic-broken-1-3| 66
.../snapshots-synthetic-broken-1-4| 73
.../snapshots-synthetic-broken-1-5| 73
.../snapshots-synthetic-broken-2-3| 43 +++
.../snapshots-synthetic-broken-2-4| 50 +++
.../snapshots-synthetic-broken-2-5| 50 +++
.../snapshots-synthetic-broken-3-4| 27 ++
.../snapshots-synthetic-broken-3-5| 27 ++
.../snapshots-synthetic-broken-4-5| 20 ++
.../blockjob-blockdev-in.xml | 4 +
39 files changed, 2290 insertions(+), 43 deletions(-)
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-2
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-3
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-2-3
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-2
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-3
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-4
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-5
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-2-3
create mode 100644 tests/qemu
This capability will be asserted once qemu stabilizes 'blockdev-reopen'.
For now we just add the capability so that we can introduce some code
that will use the reopening call. This will show our willingness to
adopt use of reopen and help qemu developers stabilize it.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_capabilities.c | 1 +
src/qemu/qemu_capabilities.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 25a77c24af..c486697d5b 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -566,6 +566,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"vhost-user-fs",
"query-named-block-nodes.flat",
"blockdev-snapshot.allow-write-only-overlay",
+ "blockdev-reopen",
);
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index e952fcb6b8..f0961e273c 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -547,6 +547,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_DEVICE_VHOST_USER_FS, /* -device vhost-user-fs */
QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT, /* query-named-block-nodes
supports the 'flat' option */
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY, /* blockdev-snapshot has the
'allow-write-only-overlay' feature */
+QEMU_CAPS_BLOCKDEV_REOPEN, /* 'blockdev-reopen' qmp command is supported */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
--
2.24.1
Set the 'id' field of the backing chain properly so that we can look
up images and initialize 6 images instead of 10 as we don't use more
currently.
Signed-off-by: Peter Krempa
---
tests/qemublocktest.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 7b7948d4c6..a6b6376c7d 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -570,6 +570,7 @@ testQemuBackupIncrementalBitmapCalculateGetFakeImage(size_t
idx)
if (!(ret = virStorageSourceNew()))
abort();
+ ret->id = idx;
ret->type = VIR_STORAGE_TYPE_FILE;
ret->format = VIR_STORAGE_FILE_QCOW2;
ret->path = g_strdup_printf("/image%zu", idx);
@@ -589,7 +590,7 @@ testQemuBackupIncrementalBitmapCalculateGetFakeChain(void)
n = ret = testQemuBackupIncrementalBitmapCalculateGetFakeImage(1);
-for (i = 2; i < 10; i++) {
+for (i = 2; i < 6; i++) {
n->backingStore =
testQemuBackupIncrementalBitmapCalculateGetFakeImage(i);
n = n->backingStore;
}
--
2.24.1
Active layer block commit makes the 'base' image the new top image of
the disk after it finishes. This means that all bitmap operations need
to be handled prior to this happening as we'd lose writes otherwise.
The ideal place is to handle it when pivoting to the new image as only
guest-writes would be happening after this point.
Use qemuBlockBitmapsHandleCommitFinish to calculate the merging
transaction.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 628fe9b107..3afdecda1b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17301,6 +17301,23 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
break;
case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
+/* we technically don't need reopen here, but we couldn't prepare
+ * the bitmaps if it wasn't present thus must skip this */
+if (blockdev &&
+virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) {
+g_autoptr(virHashTable) blockNamedNodeData = NULL;
+
+if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm,
QEMU_ASYNC_JOB_NONE)))
+return -1;
+
+if (qemuBlockBitmapsHandleCommitFinish(job->data.commit.top,
+ job->data.commit.base,
+ blockNamedNodeData,
+ &actions,
+
job->data.commit.disabledBitmapsBase) < 0)
+return -1;
+}
+
break;
}
--
2.24.1
The code deleting checkpoints needs the name of the parent checkpoint's
disk's bitmap but was using the disk alias instead. This would create
wrong bitmaps after deleting some checkpoints.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_checkpoint.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
index 5890deb471..76f10a701e 100644
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -283,7 +283,7 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
* ancestor. */
if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, parent,
chkdisk->name)))
-parentbitmap = parentchkdisk->name;
+parentbitmap = parentchkdisk->bitmap;
if (qemuCheckpointDiscardDiskBitmaps(domdisk->src, blockNamedNodeData,
chkdisk->bitmap, parentbitmap,
--
2.24.1
s/with a dot/ending with a dot/ in the summary?
On a Friday in 2020, Michal Privoznik wrote:
According to the linked BZ, machined expects either valid
hostname or valid FQDN. While in case of multiple dots, a
trailing one doesn't violate FQDN, it does violate the rule in
case of something simple, like "domain.". But it's safe to remove
it in both cases.
Please mention my libvirt commit 45464db8ba502764cf37ec9335770248bdb3d9a8
and systemd commit d65652f1f21a4b0c59711320f34266c635393c89 here.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1721804
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c | 4 ++--
tests/virsystemdtest.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
If you care about all the possible use cases:
* the above-mentioned systemd commit also forbids hyphens on either side
of a dot
* even before that commit, leading dots were not allowed
Using any of the above (including the trailing dot which this commit fixes)
is equally odd to me. (unlike the quick fix I did for the dashes, which were
a result of libvirt cutting of sensible UUIDs in half)
So even after this patch libvirt is able to generate invalid machines.
names. I simply do not care.
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
On 3/11/20 8:34 AM, Pavel Hrdina wrote:
> On Wed, Mar 11, 2020 at 01:25:50PM +0100, Ján Tomko wrote:
>> Missing blurb in the cover letter.
>>
>> On a Wednesday in 2020, Pavel Hrdina wrote:
>>> Pavel Hrdina (3):
>>> domain_conf: fix ATTRIBUTE_NONNULL for virDomainDefFormat
>>> libvirt_internal: fix ATTRIBUTE_NONNULL for virStateInitialize
>>> vircommand: fix ATTRIBUTE_NONNULL for virCommandAddArg*
>>>
>>
>> Do these actually help Coverity do its job or all they're good for
>> is finding out where we failed to update the attributes?
>
> Now that I'm trying to setup coverity scans using scan.coverity.com they
> actually help coverity do its job and I managed to run into these issues
> while building libvirt using coverity.
>
> Pavel
>
Good luck with Coverity issues - if you'd like my list of work-around
hacks I don't mind sending you the list of 55 or so local patches in my
build environment. There's even more with more recent coverity releases,
but I haven't taken the time to work through them.
These types of build issues can also be seen by enabling STATIC_ANALYSIS
for a build - IOW: outside of coverity...
FWIW: I've tried this for vircommand.h a while ago, see:
https://www.redhat.com/archives/libvir-list/2018-December/msg00548.html
The only other one I had in my backlog of hacks around things that in
general the community hasn't accepted historically is for virnetdevtap.h
and the virNetDevTapReattachBridge prototype for argument 2 because in
the source code the @brname is used in a STREQ_NULLABLE thus it also
gets flagged.
John
On Wed, Mar 11, 2020 at 09:23:50AM -0400, John Ferlan wrote:
>
>
> On 3/11/20 8:34 AM, Pavel Hrdina wrote:
> > On Wed, Mar 11, 2020 at 01:25:50PM +0100, Ján Tomko wrote:
> >> Missing blurb in the cover letter.
> >>
> >> On a Wednesday in 2020, Pavel Hrdina wrote:
> >>> Pavel Hrdina (3):
> >>> domain_conf: fix ATTRIBUTE_NONNULL for virDomainDefFormat
> >>> libvirt_internal: fix ATTRIBUTE_NONNULL for virStateInitialize
> >>> vircommand: fix ATTRIBUTE_NONNULL for virCommandAddArg*
> >>>
> >>
> >> Do these actually help Coverity do its job or all they're good for
> >> is finding out where we failed to update the attributes?
> >
> > Now that I'm trying to setup coverity scans using scan.coverity.com they
> > actually help coverity do its job and I managed to run into these issues
> > while building libvirt using coverity.
> >
> > Pavel
> >
>
> Good luck with Coverity issues - if you'd like my list of work-around
> hacks I don't mind sending you the list of 55 or so local patches in my
> build environment. There's even more with more recent coverity releases,
> but I haven't taken the time to work through them.
Thanks, I know that it will probably take some time to process all the
issues. Most of them are false positive, however, the tool I'm planning
to use, scan.coverity.com, has a possibility to provide "modeling file"
where you can write simple C-code like functions to help coverity with
the false positives.
That would be nice to see the list of patches that you already have,
maybe they could be somehow used to create the modeling file.
> These types of build issues can also be seen by enabling STATIC_ANALYSIS
> for a build - IOW: outside of coverity...
>
> FWIW: I've tried this for vircommand.h a while ago, see:
>
> https://www.redhat.com/archives/libvir-list/2018-December/msg00548.html
>
> The only other one I had in my backlog of hacks around things that in
> general the community hasn't accepted historically is for virnetdevtap.h
> and the virNetDevTapReattachBridge prototype for argument 2 because in
> the source code the @brname is used in a STREQ_NULLABLE thus it also
> gets flagged.
In this specific case it's not even coverity issue but build issue with
STATIC_ANALYSIS enabled. There are two possible solutions, removing the
ATTRIBUTE_NONNULL or disabling nonnull-compare gcc check for broken gcc.
I'm OK with both solutions.
I'm not planning to introduce any hacks into our code-base except for
things like sa_assert() which we already use.
Thanks,
Pavel
signature.asc
Description: PGP signature
On a Monday in 2020, Peter Krempa wrote:
The comment mentioned that the function resets migration params, but
that is not true.
Not true as of commit eb54cb473a8d140e0dd4a7bd42e8bcd72b056368
qemu: Reset all migration parameters
Also, you remove "free the secinfo" from the comment, even though it
still does free it.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_migration_params.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index 8c552ab9a0..f9bc43afee 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -1061,7 +1061,7 @@ qemuMigrationParamsDisableTLS(virDomainObjPtr vm,
* @apiFlags: API flags used to start the migration
*
* Deconstruct all the setup possibly done for TLS - delete the TLS and
- * security objects, free the secinfo, and reset the migration params to "".
+ * security objects.
*/
static void
qemuMigrationParamsResetTLS(virQEMUDriverPtr driver,
--
2.24.1
With the secinfo change mentioned in the commit message:
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
On a Monday in 2020, Peter Krempa wrote:
Previously qemuDomainSecretAESSetup was looking up the secret in the
secret diver as well as encrypting it for use with qemu. Split out the
the lookup into a wrapper for this function so that we can reuse the
original internals when we don't need to look up a secret with the
secret driver. The new wrapper is called
qemuDomainSecretAESSetupFromSecret.
This refactor also changes the functions to return qemuDomainSecretInfoPtr
also
Please split the split from refactors.
directly rather than filling it via an argument. This rendered
qemuDomainSecretInfoNew obsolete and thus it was deleted.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 180 ++---
src/qemu/qemu_domain.h | 2 +
2 files changed, 81 insertions(+), 101 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 10d6264e46..202b85e39a 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1035,6 +1035,8 @@ bool
qemuDomainSupportsEncryptedSecret(qemuDomainObjPrivatePtr priv);
void qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr secinfo)
ATTRIBUTE_NONNULL(1);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuDomainSecretInfo, qemuDomainSecretInfoFree);
+
You really should not need to declare a new cleanup function when
splitting code.
Jano
void qemuDomainSecretInfoDestroy(qemuDomainSecretInfoPtr secinfo);
void qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
--
2.24.1
signature.asc
Description: PGP signature
On a Monday in 2020, Peter Krempa wrote:
qemuAliasForSecret is meant as a replacement qemuDomainGetSecretAESAlias
with saner API. The sub-type we are creating the alias for is passed in
as a string rather than the unflexible 'isLuks' boolean.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_alias.c | 17 +
src/qemu/qemu_alias.h | 3 +++
2 files changed, 20 insertions(+)
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
On a Monday in 2020, Peter Krempa wrote:
In order to be able to change the function generating the alias and thus
also the aliases itself, we must hardcode the old format for the case of
upgrading form libvirt which didn't record them in the status XML yet.
Note that this code path is tested by
'tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml'
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
On 3/10/20 10:54 AM, Peter Krempa wrote:
Currently upstream commit 7f368aed67211798 + patches from this series:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg02585.html
I'll update once more to pick up the proper upstream commit for the
patches mentioned above.
---
I guess omitting S-o-b was another way to remind you of that.
Looks like it will be landing in qemu RSN™️ (real soon now):
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg03243.html
Assuming properly updated commit message,
Reviewed-by: Eric Blake
.../caps_5.0.0.x86_64.replies | 2668 +
.../caps_5.0.0.x86_64.xml |2 +-
2 files changed, 1368 insertions(+), 1302 deletions(-)
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
index 5ffa795108..7e6d711f96 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
@@ -21,7 +21,7 @@
"minor": 2,
"major": 4
},
-"package": "v4.2.0-1858-gdb736e0437"
+"package": "v4.2.0-2265-g67923a7ea6"
Don't know if you'll want to tweak this line (by one more regeneration)
once Kevin's PR actually lands.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/10/20 10:54 AM, Peter Krempa wrote:
Move the check whether the job is already synchronised to the beginning
of the function so that we don't try to do some of the steps necessary
for pivoting prior to actually wanting to pivot.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/10/20 10:54 AM, Peter Krempa wrote:
For a long time we've masked out VIR_DOMAIN_BLOCK_COPY_SHALLOW if
there's no backing chain for the copied disk to simplify the code.
One of the refacors of caused that we no longer update the 'flags'
s/refacors of/refactors of XXX/ (typo, plus figuring out what you meant
for XXX)
variable just the local copies. This was okay until in ccd4228afff I
started storing the job flags in the block job data.
Given that we modify how we call qemu we also should modify @flags so
that the correct value is recorded in the block job data.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/10/20 10:54 AM, Peter Krempa wrote:
The capability is based on qemu's support of using blockdev-snapshot to
install backing chain also for images which are in use by a block-copy
job.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 +
3 files changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a75ca0574d..25a77c24af 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -565,6 +565,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 355 */
"vhost-user-fs",
"query-named-block-nodes.flat",
+ "blockdev-snapshot.allow-write-only-overlay",
);
@@ -1442,6 +1443,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsQMPSchemaQueries[] = {
{ "human-monitor-command/$savevm-monitor-nodes",
QEMU_CAPS_SAVEVM_MONITOR_NODES },
{ "blockdev-add/arg-type/+nvme", QEMU_CAPS_DRIVE_NVME },
{ "query-named-block-nodes/arg-type/flat",
QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT },
+{ "blockdev-snapshot/$allow-write-only-overlay",
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY },
Matches the name in the qemu pull request. Long line, but not the first
such long line.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/10/20 10:54 AM, Peter Krempa wrote:
oVirt used a quirk in the pre-blockdev semantics of drive-mirror which
opened the backing chain of the mirror destination only once
'block-job-complete' was called.
Our introduction of blockdev made qemu open the backing chain images
right at the start of the job. This broke oVirt's usage of this API
because they copy the data into the backing chain during the time the
block copy job is running.
Re-introduce late open of the backing chain if qemu allows to use
reminder: "allows to ${verb}" is not idiomatic; I'd suggest either
"allows us to use" or "allows the use of"
blockdev-snapshot on write-only nodes as it can be used to install the
backing chain even for an existing image now.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 57 +++---
1 file changed, 53 insertions(+), 4 deletions(-)
@@ -17273,6 +17276,27 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
blockNamedNodeData,
shallow, &actions) < 0)
return -1;
+
+/* Open and install the backing chain of 'mirror' late if we can
use
+ * blockdev-snapshot to do it. This is to appease oVirt that wants
+ * to copy data into the backing chain while the top image is being
+ * copied shallow */
+if (reuse && shallow &&
+virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY) &&
+virStorageSourceHasBacking(disk->mirror)) {
+
+if (!(chainattachdata =
qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->mirror->backingStore,
+
priv->qemuCaps)))
+return -1;
Long lines, but I'm not sure how to improve the situation.
The comments are good at explaining the situation - we have a window of
qemu releases that regress when using -blockdev, but as long as oVirt
can force either the old -drive behavior or insist on new-enough libvirt
coupled with new-enough qemu that restores the write-only-reopen feature
that we need, then oVirt won't hit the regression. I'm not sure how you
plan to advertise to oVirt if this is a new-enough libvirt + detection
of new-enough qemu to tell oVirt they don't need to cobble libvirt into
using -drive rather than -blockdev (they might solve that by minimum
required versions, rather than having to ask libvirt), but answering
that question doesn't interfere with the validity of this patch.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
The virQEMUCaps structure has usedQMP member which in the past
used to tell if qemu we are dealing with is capable of QMP. Well,
we don't support HMP anymore (minus a few HMP passthrough
commands, which are wrapped into QMP anyways) and the member is
not used really.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_capabilities.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a75ca0574d..1395549b2b 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -638,7 +638,6 @@ static void virQEMUDomainCapsCacheDispose(void *obj)
struct _virQEMUCaps {
virObject parent;
-bool usedQMP;
bool kvmSupportsNesting;
char *binary;
@@ -1825,7 +1824,6 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps)
return NULL;
ret->invalidation = qemuCaps->invalidation;
-ret->usedQMP = qemuCaps->usedQMP;
ret->kvmSupportsNesting = qemuCaps->kvmSupportsNesting;
ret->ctime = qemuCaps->ctime;
@@ -4980,7 +4978,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
qemuCaps->version = major * 100 + minor * 1000 + micro;
qemuCaps->package = g_steal_pointer(&package);
-qemuCaps->usedQMP = true;
if (virQEMUCapsInitQMPArch(qemuCaps, mon) < 0)
return -1;
--
2.24.1
On Sat, Mar 07, 2020 at 07:30:59PM +0800, Zhang Bo wrote:
> v1:
> https://www.redhat.com/archives/libvir-list/2020-February/msg00370.html
>
> v2:
> according to Dienial's suggestion:
> * update each tls file one time -> update all of them at one time
> * forced to re-create the credentials object, rather than allowing
> to append to the original ones.
Aside from some minor mistakes this all looks fine code wise.
The commits, however, are missing the signed-off-by statement.
This is required to indicate that you agree to the contribution
policy at:
https://developercertificate.org/
Assuming you're find with this, just reply to this mail with
a Signed-off-by: YOUR NAME and I'll add this to
the commit messages & push to git with the minor fixes.
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 :|
On a Sunday in 2020, Lan wrote:
One of the BiteSizedTasks
Introduce src/util/virdaemon.c/h files
Introduce a new function virDaemonSetupLogging (src/util/virdaemon.c)
for shared code in
virLockDaemonSetupLogging (src/locking/lock_daemon.c)
virLogDaemonSetupLogging (src/logging/log_daemon.c)
daemonSetupLogging (src/remote/remote_daemon.c)
Introduce virDaemonLogConfig struct for config->log_* variables in
virLockDaemonConfig/virLogDaemonConfig/daemonConfig struct
Signed-off-by: Lan Bai
---
src/libvirt_private.syms | 2 +
src/locking/lock_daemon.c | 58 ++---
src/logging/log_daemon.c | 50 ++---
src/remote/remote_daemon.c | 57 ++---
src/util/Makefile.inc.am | 4 +-
src/util/virdaemon.c | 75 ++
src/util/virdaemon.h | 35 ++
7 files changed, 126 insertions(+), 155 deletions(-)
create mode 100644 src/util/virdaemon.c
create mode 100644 src/util/virdaemon.h
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index de0c7a3133..50cbd6d7af 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1906,6 +1906,8 @@ virCryptoHashBuf;
virCryptoHashString;
virCryptoHaveCipher;
+# util/virdaemon.h
+virDaemonSetupLogging;
# util/virdbus.h
virDBusCallMethod;
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 5e5a0c1089..5ba851cb55 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -46,6 +46,7 @@
#include "virstring.h"
#include "virgettext.h"
#include "virenum.h"
+#include "virdaemon.h"
#include "locking/lock_daemon_dispatch.h"
#include "locking/lock_protocol.h"
@@ -477,59 +478,6 @@ virLockDaemonErrorHandler(void *opaque G_GNUC_UNUSED,
}
-/*
- * Set up the logging environment
- * By default if daemonized all errors go to the logfile libvirtd.log,
- * but if verbose or error debugging is asked for then also output
- * informational and debug messages. Default size if 64 kB.
- */
-static int
-virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
- bool privileged,
- bool verbose,
- bool godaemon)
-{
-virLogReset();
-
-/*
- * Libvirtd's order of precedence is:
- * cmdline > environment > config
- *
- * Given the precedence, we must process the variables in the opposite
- * order, each one overriding the previous.
- */
-if (config->log_level != 0)
-virLogSetDefaultPriority(config->log_level);
-
-/* In case the config is empty, both filters and outputs will become empty,
- * however we can't start with empty outputs, thus we'll need to define and
- * setup a default one.
- */
-ignore_value(virLogSetFilters(config->log_filters));
-ignore_value(virLogSetOutputs(config->log_outputs));
-
-/* If there are some environment variables defined, use those instead */
-virLogSetFromEnv();
-
-/*
- * Command line override for --verbose
- */
-if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO))
-virLogSetDefaultPriority(VIR_LOG_INFO);
-
-/* Define the default output. This is only applied if there was no setting
- * from either the config or the environment.
- */
-virLogSetDefaultOutput("virtlockd", godaemon, privileged);
-
-if (virLogGetNbOutputs() == 0)
-virLogSetOutputs(virLogGetDefaultOutput());
-
-return 0;
-}
-
-
-
/* Display version information. */
static void
virLockDaemonVersion(const char *argv0)
@@ -1186,7 +1134,9 @@ int main(int argc, char **argv) {
}
VIR_FREE(remote_config_file);
-if (virLockDaemonSetupLogging(config, privileged, verbose, godaemon) < 0) {
+if (virDaemonSetupLogging((virDaemonLogConfigPtr)(&(config->log_level)),
This still does not compile for me:
../../src/locking/lock_daemon.c:1131:31: error: cast from 'unsigned int *' to
'virDaemonLogConfigPtr' (aka 'struct _virDaemonLogConfig *') increases required
alignment from 4 to 8 [-Werror,-Wcast-align]
if (virDaemonSetupLogging((virDaemonLogConfigPtr)(&(config->log_level)),
^
The first step along with introducing the virDaemonLogConfig structure
is moving the three log_parameters into this structure.
So
struct _virLockDaemonConfig {
unsigned int log_level;
char *log_filters;
char *log_outputs;
unsigned int max_clients;
unsigned int admin_max_clients;
};
would become something like
struct _virLockDaemonConfig {
virDaemonLogConfig log_config;
unsigned int max_clients;
unsigned int admin_max_clients;
};
And a function like:
virDaemonLogConfigLoadOptions(virDaemonLogConfigPtr log_config, virConfPtr conf)
could be used to replace the three lines in every daemon's config
loader.
Jano
signature.asc
Description: PGP signature
On 3/10/20 4:42 PM, Andrea Bolognani wrote:
On Mon, 2020-03-09 at 18:04 +, Daniel P. Berrangé wrote:
On Mon, Mar 09, 2020 at 06:09:13PM +0100, Andrea Bolognani wrote:
On Fri, 2020-03-06 at 17:49 +, Daniel P. Berrangé wrote:
On Fri, Mar 06, 2020 at 06:24:15PM +0100, Andrea Bolognani wrote:
[...]
Aside: instead of a per-VM setting, would it make sense for this to
be a connection-level setting? That way, even on traditional libvirt
deployments, the hypervisor admin could eg. opt out of machinectl
registration if they so desire; at the same time, you'd avoid the
situation where most VMs are confined using CGroups but a few are
not, which is probably not a desirable scenario.
Yes, functionally it would work fine as a connection level setting
too, though this hides the behaviour from the anyone looking at the
guest config. We've previously punted quite a few things to the
qemu.conf because we didn't want to go through process of representing
them in the domain XML. This was OK when the qemu.conf settings were
something done once at host deployment time.
With the embedded driver, I think this is not so desirable, as means
to get the configuration they want from a VM, they need to deal with
two distinct config files. The ideal would be that everything that
is commonly needed can be achieved solely in the domain XML, and
I think resource control backend is one such common tunable.
I don't have a strong opinion either way, and as far as my current
use is concerned it doesn't bother me to have to deal with a second
configuration file. The reason why I thought a per-VM setting might
not be desirable is that applications would then be able to
override it, and so maybe VMs created with virt-manager would be
registered against machinectl but VMs created using GNOME Boxes
would not, and if the sysadmin likes to use machinectl to get a
comprehensive view of the system they'd no longer be guaranteed
that. But if that's not the kind of scenario we think we should
prevent, then a per-VM setting is fine by me :)
I still don't quite see the value in machinectl (maybe because I'm not
using systemd :-D), but anyway - it's a system-wide monitor of virtual
machines. Therefore it makes sense to register a domain started under
qemu:///embed there. I don't view embed mode as a way of starting VMs
secretly. It's a way of starting VMs privately and that's a different
thing. Other users might learn that my app is running a VM (plain 'ps'
would give it away), but they can not mangle with it in any way, e.g.
change its XML.
[...]
Right now we're already doing
qemu-$domid-$domname
where I'm not entirely sure how much $domid actually buys us.
IIRC $domid was a hack because at one time we had problems with
systemd not cleaning up the transient scope when QEMU was killed.
This would prevent libvirt starting the guest again thereafter.
I can't remember now if this was a bug we fixed in systemd or
libvirt or both or neither.
It was introduced by bd773e74f0d1d1b9ebbfcaa645178316b4f2265c and the
commit message links to this bug:
https://bugs.freedesktop.org/show_bug.cgi?id=68370 which looks like
fixed in systemd.
I see! It would be neat if we could get rid of it, assuming of course
it's no longer needed on the platforms we target.
I don't think it's that simple. Machinectl poses some limitations on the
name: either it has to be a FQDN or a simple name without any dots. And
according to our code the strlen() must be <= 64 (don't know if that
comes from machinectl or is just our own limitation). Therefore, if you
have two domains which names would clash after our processing, the
domain ID guarantees unique strings are passed to machined.
[...]
Of course you can turn off virtlogd via qemu.conf
That's what I'm doing right now and it works well enough, but I'm
afraid that requiring a bunch of setup will discourage developers
from using the embedded driver. We should aim for a reasonable out
of the box experience instead.
Why do you need to turn it off though ? It should already
"do the right thing" as the log files should appear under a
different location and not have any clash. Turning it off
immediately creates a denial of service CVE in your application.
I was getting the same SELinux denial that Michal reported a few
days back: virtlogd wants to verify it's being connected to by a
process running as root, but it's only allowed by the policy to
look into libvirtd's /proc/$pid for this information. So, for the
same reason virtqemud can't currently connect to virtlogd when
SELinux is in enforcing mode, neither can my qemu:///embed-using
application.
Besides that, there is the fact that a lot of people, mainly those
coming from a containers background, are not happy with having extra
daemons running. I'm not saying they would prefer being hit by a DoS
than having virtlogd running :) but they really, really don't like
daemons :)
None the less, as above I think we need common things
On 3/11/20 7:55 AM, Peter Krempa wrote:
This capability will be asserted once qemu stabilizes 'blockdev-reopen'.
For now we just add the capability so that we can introduce some code
that will use the reopening call. This will show our willingness to
adopt use of reopen and help qemu developers stabilize it.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_capabilities.c | 1 +
src/qemu/qemu_capabilities.h | 1 +
2 files changed, 2 insertions(+)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
Introduce the monitor code for using blockdev-reopen. For now we'll use
x-blockdev-reopen so that the interactions between qemu and libvirt
can be tested with the existing code.
Since the usage will be guarded by the for-now unasserted capability
we'll be able to change the called command when the command will be
stabilized.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor.c | 13 +
src/qemu/qemu_monitor.h | 3 +++
src/qemu/qemu_monitor_json.c | 21 +
src/qemu/qemu_monitor_json.h | 4
4 files changed, 41 insertions(+)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
Introduce a set of helpers to call blockdev-reopen in certain scenarios
Libvirt will use the QMP command to turn certain members of the backing
chain read-write for bitmap manipulation and we'll also want to use it
to replace/install the backing chain of a qcow2 format node.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 101 ++
src/qemu/qemu_block.h | 9
2 files changed, 110 insertions(+)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
Qemu's bitmap APIs don't reopen the appropriate images read-write for
modification. It's libvirt's duty to reopen them via blockdev-reopen
if we wish to modify the bitmaps.
Use the new helpers to reopen the images for bitmap manipulation.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_checkpoint.c | 7 +++
1 file changed, 7 insertions(+)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
The code deleting checkpoints needs the name of the parent checkpoint's
disk's bitmap but was using the disk alias instead. This would create
wrong bitmaps after deleting some checkpoints.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_checkpoint.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
index 5890deb471..76f10a701e 100644
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -283,7 +283,7 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
* ancestor. */
if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, parent,
chkdisk->name)))
-parentbitmap = parentchkdisk->name;
+parentbitmap = parentchkdisk->bitmap;
Shouldn't this go in now regardless of whether the rest of the series is
still waiting on qemu?
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
Since capabilities are not present for inactive VMs we'd report that we
don't support '--delete' or commiting while checkpoints exist rather
committing
than the proper error.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
I'll be adding more fields to care about so splitting the code out will
be better long-term.
Signed-off-by: Peter Krempa
Reviewed-by: Pavel Mores
---
src/qemu/qemu_domain.c | 27 +++
1 file changed, 19 insertions(+), 8 deletions(-)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
I'll be adding more fields to care about so splitting the code out will
be better long-term.
Signed-off-by: Peter Krempa
Reviewed-by: Pavel Mores
---
src/qemu/qemu_domain.c | 57 ++
1 file changed, 36 insertions(+), 21 deletions(-)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
Starting a commit job will require disabling bitmaps in the base image
so that they are not dirtied by the commit job. We need to store a list
of the bitmaps so that we can later re-enable them.
I think I get why you don't want them further dirtied, but does it
really matter?
Visually, consider what happens if we create checkpoint1 after writing
data A, then checkpoint2 after writing data B (rendering the first
checkpoing read-only) before writing data C, then create an external
snapshot (possibly simultaneous with checkpoint3) to create a new active
image (the new image also has a distinct bitmap tracking all changes
since the snapshot, while the snapshot in the now-backing image remains
read-write but unchanged because we no longer write to the backing)
before writing data D:
backing: AABBCC--
bitmap1: --11 (disabled)
bitmap2: 11-- (enabled)
active:---D-DD-
bitmap3: ---1-11- (enabled)
In the above, I used different bitmap names between backing and active
(as that is less confusing), but I don't know if that's what your code
for bitmaps + external snapshots currently does, or if we end up with
two bitmaps both named bitmap2 in both images; but that shouldn't really
matter (as qemu needs both the node name and a bitmap name). While in
this configuration, a differential backup from checkpoint1 uses
backing/bitmap1 + backing/bitmap2 + active/bitmap3 == --1-; a
differential from checkpoint2 uses backing/bitmap2 + active/bitmap3 ==
----; a differential from the time of the external snapshot creation
uses active/bitmap3 == ---1-11-.
So, the question is what happens if we now want to commit active back
into backing. If we leave bitmap2 enabled while the commit happens, we
end up with:
backing: AABDCDD-
bitmap1: --11 (disabled)
bitmap2: ---- (enabled)
You can no longer recreate the point in time where the external snapshot
was created (the commit lost checkpoint3), but bitmap2 is still viable
for all changes needed for an incremental backup of changes made after
bitmap1 was disabled and bitmap2 created.
Conversely, if we disable bitmap2 prior to the commit, then you have the
option of creating a new bitmap3 prior to the commit to end up with:
backing: AABDCDD-
bitmap1: --11 (disabled)
bitmap2: 11-- (disabled)
bitmap3: ---1-11- (enabled)
such that you now have a point-in-time where you can then do a
differential backup from all changes after the external snapshot was
created (even though it has now been committed). But your resulting
backups from any of the checkpoints see the same cumulative bitmaps as
before.
And what happens if you accidentally leave bitmap2 enabled during the
commit?
backing: AABDCDD-
bitmap1: --11 (disabled)
bitmap2: ---- (disabled)
bitmap3: ---1-11- (enabled)
You have recorded more bits than strictly necessary in bitmap2, but as
bitmap2 is unusable on its own (a differential bitmap HAS to combine
bitmap2 with bitmap3, whether you are doing the differential from
checkpoint1 or checkoint2), and all of those same bits are recorded in
bitamp3, the resulting cumulative bitmap is unchanged.
Thus, my question is whether this complexity is necessary because it
buys us some safety (is there some failure path I'm overlooking, where
keeping bitmap2 unchanged even though backing is partially modified
before the failure of a half-completed commit is essential?), but I can
still go ahead and review the patch on the assumption that you have a
reason for not allowing a stranded-enabled bitmap after a snapshot
operation to not be modified during commit.
Add a field and status XML handling code as well as a test.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_blockjob.h | 2 ++
src/qemu/qemu_domain.c| 26 +++
.../blockjob-blockdev-in.xml | 4 +++
3 files changed, 32 insertions(+)
At any rate, I'm not seeing any functional problems with the added code,
even if I still have a question on the technical need.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
Set the 'id' field of the backing chain properly so that we can look
up images and initialize 6 images instead of 10 as we don't use more
s/images/images,/
currently.
Signed-off-by: Peter Krempa
---
tests/qemublocktest.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
qemuBlockBitmapsHandleCommitStart prepares for disabling the bitmaps in
the 'base' of the commit job so that the bitmaps are not dirtied by the
commit job. This needs to be done prior to start of the commit job.
qemuBlockBitmapsHandleCommitFinish then calculates the necessary merges
that agregate all the bitmaps between the commited images and write them
into the base bitmap.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 219 ++
src/qemu/qemu_block.h | 14 +++
2 files changed, 233 insertions(+)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index edebbcd0ce..6853c021ca 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -2962,6 +2962,225 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src,
}
+/**
+ * @topsrc: virStorageSource representing 'top' of the job
+ * @basesrc: virStorageSource representing 'base' of the job
+ * @blockNamedNodeData: hash table containing data about bitmaps
+ * @actions: filled with arguments for a 'transaction' command
+ * @disabledBitmapsBase: filled with a list of bitmap names which must be
disabled
+ *
+ * Prepares data for correctly hanlding bitmaps during the start of a commit
handling
+ * job. The bitmaps in the 'base' image must be disabled, so that the writes
+ * done by the blockjob don't dirty the enabled bitmaps.
+ *
+ * @actions and @disabledBitmapsBase are untouched if no bitmaps need
+ * to be disabled.
+ */
+int
+qemuBlockBitmapsHandleCommitStart(virStorageSourcePtr topsrc,
+ virStorageSourcePtr basesrc,
+ virHashTablePtr blockNamedNodeData,
+ virJSONValuePtr *actions,
+ char ***disabledBitmapsBase)
+{
+static int
+qemuBlockBitmapsHandleCommitFinishIterate(void *payload,
+ const void *entryname,
+ void *opaque)
+{
+struct qemuBlockBitmapsHandleCommitData *data = payload;
+const char *bitmapname = entryname;
+virJSONValuePtr actions = opaque;
+
+if (data->skip)
+return 0;
+
+if (data->create) {
+if (qemuMonitorTransactionBitmapAdd(actions, data->basenode,
bitmapname,
+data->persistent, !data-> enable,
Spurious space.
+/**
+ * @topsrc: virStorageSource representing 'top' of the job
+ * @basesrc: virStorageSource representing 'base' of the job
+ * @blockNamedNodeData: hash table containing data about bitmaps
+ * @actions: filled with arguments for a 'transaction' command
+ * @disabledBitmapsBase: bitmap names which were disabled
+ *
+ * Calculates the necessary bitmap merges/additions/enablements to properly
+ * handle commit of images from 'top' into 'base'. The necessary operations
+ * in form of argumets of the 'transaction' command are filled into 'actions'
typo and grammar:
in the form of arguments to
+ * if there is anything to do. Otherwise NULL is returned.
+ */
+int
+qemuBlockBitmapsHandleCommitFinish(virStorageSourcePtr topsrc,
+ virStorageSourcePtr basesrc,
+ virHashTablePtr blockNamedNodeData,
+ virJSONValuePtr *actions,
+ char **disabledBitmapsBase)
+{
+++ b/src/qemu/qemu_block.h
@@ -229,6 +229,20 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src,
bool shallow,
virJSONValuePtr *actions);
+int
+qemuBlockBitmapsHandleCommitStart(virStorageSourcePtr topsrc,
+ virStorageSourcePtr basesrc,
+ virHashTablePtr blockNamedNodeData,
+ virJSONValuePtr *actions,
+ char ***disabledBitmapsBase);
+
+int
+qemuBlockBitmapsHandleCommitFinish(virStorageSourcePtr topsrc,
+ virStorageSourcePtr basesrc,
+ virHashTablePtr blockNamedNodeData,
+ virJSONValuePtr *actions,
+ char **disabledBitmapsBase);
+
int
qemuBlockReopenReadWrite(virDomainObjPtr vm,
virStorageSourcePtr src,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
Add code for testing the two necessary steps of handling bitmaps during
block commit and excercise the code on the test data which we have for
exercise
bitmap handling.
Signed-off-by: Peter Krempa
---
tests/qemublocktest.c | 95 ++
.../bitmapblockcommit/basic-1-2 | 119 ++
.../bitmapblockcommit/basic-1-3 | 119 ++
.../bitmapblockcommit/basic-2-3 | 2 +
4 files changed, 335 insertions(+)
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-2
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-3
create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-2-3
Impacted if my concerns earlier in the series about even needing to
pre-disable bitmaps prior to a commit causes a change. But without a
design change, this looks reasonable for covering the code just added.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:55 AM, Peter Krempa wrote:
Use the 'snapshots-synthetic-broken' test data for block-commit.
Signed-off-by: Peter Krempa
---
+++ b/tests/qemublocktest.c
@@ -1305,6 +1305,19 @@ mymain(void)
TEST_BITMAP_BLOCKCOMMIT("snapshots-4-5", 4, 5, "snapshots");
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-2", 1, 2,
"snapshots-synthetic-broken");
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-3", 1, 3,
"snapshots-synthetic-broken");
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-4", 1, 4,
"snapshots-synthetic-broken");
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-5", 1, 5,
"snapshots-synthetic-broken");
+
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-2-3", 2, 3,
"snapshots-synthetic-broken");
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-2-4", 2, 4,
"snapshots-synthetic-broken");
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-2-5", 2, 5,
"snapshots-synthetic-broken");
+
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-3-4", 3, 4,
"snapshots-synthetic-broken");
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-3-5", 3, 5,
"snapshots-synthetic-broken");
+
+TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-4-5", 4, 5,
"snapshots-synthetic-broken");
Again, may be impacted if we change the design, but the test looks like
good coverage of the design that the rest of this series adds.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:56 AM, Peter Krempa wrote:
Add an argument to qemuBlockJobDiskNewCommit to propagate the list of
disabled bitmaps into the job data structure.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_blockjob.c | 2 ++
src/qemu/qemu_blockjob.h | 1 +
src/qemu/qemu_driver.c | 2 +-
3 files changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
changes in v2, all of them affecting just pSeries guests:
- added 'label' requirement
- added code to align down the NVDIMM device
previous version:
https://www.redhat.com/archives/libvir-list/2020-March/msg00165.html
This patch series adds NVDIMM suport for ppc64 guests,
which consists on adding an extra 'uuid' element in
the nvdimm command line and the target label size must
always be provided in the memory definition.
No changes were made in the existing NVDIMM support for
x86 and other archs.
Daniel Henrique Barboza (5):
conf: Introduce optional 'uuid' element for NVDIMM memory
formatdomain.html.in: document the new 'uuid' NVDIMM element
conf, qemu: enable NVDIMM support for ppc64
formatdomain.html.in: document NVDIMM 'label' requirement for pSeries
news.xml: document the new NVDIMM support for Pseries guests
docs/formatdomain.html.in | 24 +++--
docs/news.xml | 11
docs/schemas/domaincommon.rng | 5 ++
src/conf/domain_conf.c| 44 ++--
src/conf/domain_conf.h| 3 ++
src/qemu/qemu_command.c | 7 +++
src/qemu/qemu_domain.c| 47 +++--
.../memory-hotplug-nvdimm-ppc64.args | 32
.../memory-hotplug-nvdimm-ppc64.xml | 50 +++
tests/qemuxml2argvtest.c | 4 ++
.../memory-hotplug-nvdimm-ppc64.xml | 50 +++
tests/qemuxml2xmltest.c | 2 +
12 files changed, 268 insertions(+), 11 deletions(-)
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml
create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
--
2.24.1
Signed-off-by: Daniel Henrique Barboza
---
docs/news.xml | 11 +++
1 file changed, 11 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml
index 7fd88f9998..6eb222cdb7 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -44,6 +44,17 @@
+
+
+ qemu: NVDIMM support for pSeries guests
+
+
+ QEMU 5.0 implements NVDIMM memory support for pSeries guests. This
+ is done by adding an 'uuid' element in the memory XML, which can
+ either be provided in the XML or, if ommited, generated
+ automatically.
+
+
--
2.24.1
Signed-off-by: Daniel Henrique Barboza
---
docs/formatdomain.html.in | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index e3bf33f873..142cd17205 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -9026,12 +9026,13 @@ qemu-kvm -net nic,model=? /dev/null
label
- For NVDIMM type devices one can optionally use
- label and its subelement size
- to configure the size of namespaces label storage
- within the NVDIMM module. The size element
- has usual meaning described
+ For NVDIMM type devices one can use label and its
+ subelement size to configure the size of
+ namespaces label storage within the NVDIMM module. The
+ size element has usual meaning described
here.
+ label is mandatory for pSeries guests and optional
+ for all other architectures.
For QEMU domains the following restrictions apply:
--
2.24.1
Using the 'uuid' element for ppc64 NVDIMM memory added in the
previous patch, use it in qemuBuildMemoryDeviceStr() to pass
it over to QEMU.
Another ppc64 restriction is the necessity of a mem->labelsize,
given than ppc64 only support label-area backed NVDIMMs.
Finally, we don't want ppc64 NVDIMMs to align up due to the
high risk of going beyond the end of file with a 256MiB
increment that the user didn't predict. Align it down
instead.
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c| 7 +++
src/qemu/qemu_command.c | 7 +++
src/qemu/qemu_domain.c| 47 +--
.../memory-hotplug-nvdimm-ppc64.args | 32 +
.../memory-hotplug-nvdimm-ppc64.xml | 5 +-
tests/qemuxml2argvtest.c | 4 ++
.../memory-hotplug-nvdimm-ppc64.xml | 5 +-
7 files changed, 102 insertions(+), 5 deletions(-)
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3ae6c181c2..7f8018fed2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16582,6 +16582,13 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr
xmlopt,
if (virDomainMemoryTargetDefParseXML(node, ctxt, def) < 0)
goto error;
+if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
+ARCH_IS_PPC64(dom->os.arch) && def->labelsize == 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("label size is required for NVDIMM device"));
+goto error;
+}
+
if (virDomainDeviceInfoParseXML(xmlopt, memdevNode,
&def->info, flags) < 0)
goto error;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9e0334a3e7..76f1247329 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3677,6 +3677,13 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
if (mem->labelsize)
virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);
+if (virUUIDIsValid(mem->uuid)) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+virUUIDFormat(mem->uuid, uuidstr);
+virBufferAsprintf(&buf, "uuid=%s,", uuidstr);
+}
+
if (mem->readonly) {
if (!virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_DEVICE_NVDIMM_UNARMED)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3d3f796d85..2f420a43cd 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12418,6 +12418,35 @@ qemuDomainGetMemoryModuleSizeAlignment(const
virDomainDef *def,
}
+static void
+qemuDomainNVDimmAlignSizePseries(virDomainDefPtr def,
+ virDomainMemoryDefPtr mem)
+{
+/* For NVDIMMs in ppc64 in we want to align down the guest
+ * visible space, instead of align up, to avoid writing
+ * beyond the end of file by adding a potential 256MiB
+ * to the user specified size.
+ *
+ * The label-size is mandatory for ppc64 as well, meaning that
+ * the guest visible space will be target_size-label_size.
+ *
+ * Finally, target_size must include label_size.
+ *
+ * The above can be summed up as follows:
+ *
+ * target_size = AlignDown(target_size - label_size) + label_size
+ */
+unsigned long long ppc64AlignSize = qemuDomainGetMemorySizeAlignment(def);
+unsigned long long guestArea = mem->size - mem->labelsize;
+
+/* Align down guest_area. 256MiB is the minimum size. */
+guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize;
+guestArea = MAX(guestArea, ppc64AlignSize);
+
+mem->size = guestArea + mem->labelsize;
+}
+
+
int
qemuDomainAlignMemorySizes(virDomainDefPtr def)
{
@@ -12464,8 +12493,14 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def)
/* Align memory module sizes */
for (i = 0; i < def->nmems; i++) {
-align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]);
-def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align);
+if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
+ARCH_IS_PPC64(def->os.arch)) {
+qemuDomainNVDimmAlignSizePseries(def, def->mems[i]);
+} else {
+align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]);
+def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align);
+}
+
hotplugmem += def->mems[i]->size;
if (def->mems[i]->size > maxmemkb) {
@@ -12494,7 +12529,13 @@ void
qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def,
virDomainMemoryDefPtr mem)
{
-mem->size = VIR_ROUND_UP(mem->size, qemuDomainGetMemorySizeAlignment(def));
+if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
+ARCH_IS_PPC64(def->os.arch)) {
+qemuDomainNVDimmAlig
On 3/11/20 7:56 AM, Peter Krempa wrote:
On start of the commit job, we need to disable any active bitmap in the
base. Use qemuBlockBitmapsHandleCommitStart to calculate which and call
the appropriate QMP APIs. We use blockdev-reopen to make the 'base'
writable to disable the bitmaps.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 43 +-
1 file changed, 42 insertions(+), 1 deletion(-)
Someday, it would be nice if qemu let us do this without requiring us to
do the reopen step ourselves, but in the meantime, this looks like it
works. Design-wise, I'm still not convinced why we have to disable the
bitmaps in the base prior to the commit (as the only thing a bitmap is
good for is for cumulative merging in determining how much of an image
to expose over a future differential backup, but that differential is
the same whether we write bits in one or multiple bitmaps as part of the
commit operation). But code wise, this looks accurate.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:56 AM, Peter Krempa wrote:
Active layer block commit makes the 'base' image the new top image of
the disk after it finishes. This means that all bitmap operations need
to be handled prior to this happening as we'd lose writes otherwise.
The ideal place is to handle it when pivoting to the new image as only
guest-writes would be happening after this point.
Use qemuBlockBitmapsHandleCommitFinish to calculate the merging
transaction.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 628fe9b107..3afdecda1b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17301,6 +17301,23 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
break;
case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
+/* we technically don't need reopen here, but we couldn't prepare
+ * the bitmaps if it wasn't present thus must skip this */
+if (blockdev &&
+virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) {
+g_autoptr(virHashTable) blockNamedNodeData = NULL;
+
+if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm,
QEMU_ASYNC_JOB_NONE)))
+return -1;
+
+if (qemuBlockBitmapsHandleCommitFinish(job->data.commit.top,
+ job->data.commit.base,
+ blockNamedNodeData,
+ &actions,
+
job->data.commit.disabledBitmapsBase) < 0)
+return -1;
+}
+
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/11/20 7:56 AM, Peter Krempa wrote:
If a block-copy fails we should at least re-enable the bitmaps so that
the operation can be re-tried.
The subject and commit body say block-copy,
Signed-off-by: Peter Krempa
---
src/qemu/qemu_blockjob.c | 42 ++--
1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index ed7959175a..60fe1cedf6 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -1346,6 +1346,40 @@
qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver,
}
+static void
+qemuBlockJobProcessEventFailedCommitCommon(virDomainObjPtr vm,
+ qemuBlockJobDataPtr job,
+ qemuDomainAsyncJob asyncJob)
+{
but the function name say commit. Is that intended?
+qemuDomainObjPrivatePtr priv = vm->privateData;
+g_autoptr(virJSONValue) actions = virJSONValueNewArray();
+char **disabledBitmaps = job->data.commit.disabledBitmapsBase;
+
+if (!disabledBitmaps || !*disabledBitmaps)
+return;
+
+for (; *disabledBitmaps; disabledBitmaps++) {
+qemuMonitorTransactionBitmapEnable(actions,
+ job->data.commit.base->nodeformat,
+ *disabledBitmaps);
+}
+
+if (qemuBlockReopenReadWrite(vm, job->data.commit.base, asyncJob) < 0)
+return;
+
+if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0)
+return;
+
+qemuMonitorTransaction(priv->mon, &actions);
+
+if (qemuDomainObjExitMonitor(priv->driver, vm) < 0)
+return;
+
+if (qemuBlockReopenReadOnly(vm, job->data.commit.base, asyncJob) < 0)
+return;
+}
Does it really matter? The only reason a bitmap is left enabled in a
backing image when we create an external snapshot is because it was
easier to leave it alone than to temporarily reopen the backing image
read-write just to disable the bitmap. But as long as no writes to the
backing file happen (until commit), whether it is left enabled or
changed to disabled doesn't affect behavior; so we could also argue that
if we changed it to disabled prior to attempting commit, then commit
fails, it really doesn't matter if we leave it disabled rather than
trying to re-enable it (just to have it be re-disabled on the retry
attempt).
But on the grounds of trying to leave things as close to what they were
before failure, I'm okay with this patch, if you can straighten out my
confusion on naming.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 3/9/20 11:22 AM, Peter Krempa wrote:
Previously qemuDomainSecretAESSetup was looking up the secret in the
secret diver as well as encrypting it for use with qemu. Split out the
driver
the lookup into a wrapper for this function so that we can reuse the
original internals when we don't need to look up a secret with the
secret driver. The new wrapper is called
qemuDomainSecretAESSetupFromSecret.
This refactor also changes the functions to return qemuDomainSecretInfoPtr
directly rather than filling it via an argument. This rendered
qemuDomainSecretInfoNew obsolete and thus it was deleted.
Signed-off-by: Peter Krempa
---
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On Mar 11, 2020 9:30 AM, Ján Tomko wrote:
On a Sunday in 2020, Lan wrote:
>One of the BiteSizedTasks
>
>Introduce src/util/virdaemon.c/h files
>
>Introduce a new function virDaemonSetupLogging (src/util/virdaemon.c)
>for shared code in
>virLockDaemonSetupLogging (src/locking/lock_daemon.c)
>virLogDaemonSetupLogging (src/logging/log_daemon.c)
>daemonSetupLogging (src/remote/remote_daemon.c)
>
>Introduce virDaemonLogConfig struct for config->log_* variables in
>virLockDaemonConfig/virLogDaemonConfig/daemonConfig struct
>
>Signed-off-by: Lan Bai
>---
> src/libvirt_private.syms | 2 +
> src/locking/lock_daemon.c | 58 ++---
> src/logging/log_daemon.c | 50 ++---
> src/remote/remote_daemon.c | 57 ++---
> src/util/Makefile.inc.am | 4 +-
> src/util/virdaemon.c | 75 ++
> src/util/virdaemon.h | 35 ++
> 7 files changed, 126 insertions(+), 155 deletions(-)
> create mode 100644 src/util/virdaemon.c
> create mode 100644 src/util/virdaemon.h
>
>diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>index de0c7a3133..50cbd6d7af 100644
>--- a/src/libvirt_private.syms
>+++ b/src/libvirt_private.syms
>@@ -1906,6 +1906,8 @@ virCryptoHashBuf;
> virCryptoHashString;
> virCryptoHaveCipher;
>
>+# util/virdaemon.h
>+virDaemonSetupLogging;
>
> # util/virdbus.h
> virDBusCallMethod;
>diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
>index 5e5a0c1089..5ba851cb55 100644
>--- a/src/locking/lock_daemon.c
>+++ b/src/locking/lock_daemon.c
>@@ -46,6 +46,7 @@
> #include "virstring.h"
> #include "virgettext.h"
> #include "virenum.h"
>+#include "virdaemon.h"
>
> #include "locking/lock_daemon_dispatch.h"
> #include "locking/lock_protocol.h"
>@@ -477,59 +478,6 @@ virLockDaemonErrorHandler(void *opaque G_GNUC_UNUSED,
> }
>
>
>-/*
>- * Set up the logging environment
>- * By default if daemonized all errors go to the logfile libvirtd.log,
>- * but if verbose or error debugging is asked for then also output
>- * informational and debug messages. Default size if 64 kB.
>- */
>-static int
>-virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
>- bool privileged,
>- bool verbose,
>- bool godaemon)
>-{
>-virLogReset();
>-
>-/*
>- * Libvirtd's order of precedence is:
>- * cmdline > environment > config
>- *
>- * Given the precedence, we must process the variables in the opposite
>- * order, each one overriding the previous.
>- */
>-if (config->log_level != 0)
>-virLogSetDefaultPriority(config->log_level);
>-
>-/* In case the config is empty, both filters and outputs will become
>empty,
>- * however we can't start with empty outputs, thus we'll need to define
>and
>- * setup a default one.
>- */
>-ignore_value(virLogSetFilters(config->log_filters));
>-ignore_value(virLogSetOutputs(config->log_outputs));
>-
>-/* If there are some environment variables defined, use those instead */
>-virLogSetFromEnv();
>-
>-/*
>- * Command line override for --verbose
>- */
>-if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO))
>-virLogSetDefaultPriority(VIR_LOG_INFO);
>-
>-/* Define the default output. This is only applied if there was no setting
>- * from either the config or the environment.
>- */
>-virLogSetDefaultOutput("virtlockd", godaemon, privileged);
>-
>-if (virLogGetNbOutputs() == 0)
>-virLogSetOutputs(virLogGetDefaultOutput());
>-
>-return 0;
>-}
>-
>-
>-
> /* Display version information. */
> static void
> virLockDaemonVersion(const char *argv0)
>@@ -1186,7 +1134,9 @@ int main(int argc, char **argv) {
> }
> VIR_FREE(remote_config_file);
>
>-if (virLockDaemonSetupLogging(config, privileged, verbose, godaemon) < 0)
>{
>+if (virDaemonSetupLogging((virDaemonLogConfigPtr)(&(config->log_level)),
This still does not compile for me:
../../src/locking/lock_daemon.c:1131:31: error: cast from 'unsigned int *' to
'virDaemonLogConfigPtr' (aka 'struct _virDaemonLogConfig *') increases required
alignment from 4 to 8 [-Werror,-Wcast-align]
if (virDaemonSetupLogging((virDaemonLogConfigPtr)(&(config->log_level)),
^
This is modified in following commits. If you apply the whole patch it
shouldn't be a problem.
The first step along with introducing the virDaemonLogConfig structure
is moving the three log_parameters into this structure.
So
struct _virLockDaemonConfig {
unsigned int log_level;
char *log_filters;
char *log_outputs;
unsigned int max_clients;
unsigned int admin_max_clients;
};
would become something like
struct _virLockDaemonConfig {
virDaemonLogConfig log_config;
unsigned int max_clients;
unsigned int admin_max_clients;
};
And a function
