Re: [PATCH] Allow VM to read sysfs PCI config, revision files

[Christian Ehrhardt]

On Thu, May 19, 2022 at 3:04 PM Michal Prívozník  wrote:
>
> On 5/19/22 08:09, Christian Ehrhardt wrote:
> > On Thu, May 12, 2022 at 3:27 PM Max Goodhart  wrote:
> >>
> >> Oops, I didn't intend for the commit author email to be 
> >> git...@chromakode.com here. Would you please use c...@chromakode.com as 
> >> the author of the patch?
> >
> > Yeah I can amend that (and we see you control that email address).
> >
> > Also since I know this is coming from a particular LP bug I'd also add
> > the following to have the full trace-back from the commit message to
> > more context
> >
> > Fixes: https://launchpad.net/bugs/1972075
> >
> > Can I apply it that way or are there any objections?
>
> Yeah, go ahead and push. The change does indeed fix the problem.

Thanks Michal and Max,
applied!

> Michal
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [libvirt PATCH 69/80] qemu: Implement VIR_MIGRATE_POSTCOPY_RESUME for Finish phase

[Jiri Denemark]

On Thu, May 12, 2022 at 17:20:34 +0200, Peter Krempa wrote:
> On Tue, May 10, 2022 at 17:21:30 +0200, Jiri Denemark wrote:
> > Everything was already done in the normal Finish phase and vCPUs are
> > running. We just need to wait for all remaining data to be transferred.
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> >  src/qemu/qemu_migration.c | 46 ++-
> >  1 file changed, 40 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index a8481f7515..430dfb1abb 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -6600,6 +6600,22 @@ qemuMigrationDstFinishFresh(virQEMUDriver *driver,
> >  }
> >  
> >  
> > +static int
> > +qemuMigrationDstFinishResume(virQEMUDriver *driver,
> > + virDomainObj *vm)
> > +{
> > +VIR_DEBUG("vm=%p", vm);
> > +
> > +if (qemuMigrationDstWaitForCompletion(driver, vm,
> > +  VIR_ASYNC_JOB_MIGRATION_IN,
> > +  false) < 0) {
> > +return -1;
> > +}
> 
> As I mentioned in another reply, IMO it would be useful to allow
> adoption of a unattended running migration precisely for this case, so
> that mgmt apps don't have to encode more logic to wait for events if
> migration was actually running fine.

I think event processing in mgmt apps is inevitable anyway as the
migration may finish before an app even tries to recover the migration
in case only libvirt side of migration was broken. And it may still be
broken to make recovery impossible while migration is progressing just
fine (esp. if migration streams use separate network). Even plain
post-copy which never fails requires event processing since the
management has to explicitly switch migration to post-copy mode. Also
events are often the only way to get statistics about the completed
migration.

That said, I think it would be nice to have this functionality anyway so
I'll try to look at it as a follow up series.

Jirka

Re: [libvirt PATCH 68/80] qemu: Implement VIR_MIGRATE_POSTCOPY_RESUME for Prepare phase

[Jiri Denemark]

On Thu, May 12, 2022 at 17:16:32 +0200, Peter Krempa wrote:
> On Tue, May 10, 2022 at 17:21:29 +0200, Jiri Denemark wrote:
> > The QEMU process is already running, all we need to do is to call
> > migrate-recover QMP command. Except for some checks and cookie handling,
> > of course.
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> >  src/qemu/qemu_migration.c | 99 +++
> >  1 file changed, 99 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 2e9235e1d5..a8481f7515 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -3509,6 +3509,98 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver,
> >  }
> 
> [...]
> 
> > +
> > +if (qemuMigrationAnyRefreshStatus(driver, vm, 
> > VIR_ASYNC_JOB_MIGRATION_IN,
> > +  ) < 0)
> > +goto cleanup;
> > +
> > +if (status != VIR_DOMAIN_JOB_STATUS_POSTCOPY_PAUSED) {
> > +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > +   _("QEMU reports migration is still running"));
> > +goto cleanup;
> > +}
> 
> Is there any reason not to allow adopting a running unattended migration
> with a recovery API?

It's easier this way :-) The migration will happily finish anyway so it
does not really matter. You can't just have a synchronous API call and
finished migration is only announced via events. Anyway, it can
definitely be done in a follow up series if desired, although it will
make the code even more complicated as we would have to do all the steps
and just avoid running any QMP commands and jump right into waiting for
migration to finish on both sides.

Jirka

Re: [libvirt PATCH 66/80] qemu: Handle incoming migration in qemuMigrationAnyConnectionClosed

[Jiri Denemark]

On Thu, May 12, 2022 at 17:09:27 +0200, Peter Krempa wrote:
> On Tue, May 10, 2022 at 17:21:27 +0200, Jiri Denemark wrote:
> > Signed-off-by: Jiri Denemark 
> > ---
> >  src/qemu/qemu_migration.c | 10 ++
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index c0ab59f688..c1a60c90ef 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -2378,15 +2378,12 @@ qemuMigrationAnyConnectionClosed(virDomainObj *vm,
> >  case QEMU_MIGRATION_PHASE_POSTCOPY_FAILED:
> >  case QEMU_MIGRATION_PHASE_BEGIN_RESUME:
> >  case QEMU_MIGRATION_PHASE_PERFORM_RESUME:
> > +case QEMU_MIGRATION_PHASE_PREPARE_RESUME:
> >  VIR_DEBUG("Connection closed while resuming failed post-copy 
> > migration");
> >  postcopy = true;
> >  break;
> >  
> >  case QEMU_MIGRATION_PHASE_PREPARE:
> > -case QEMU_MIGRATION_PHASE_FINISH2:
> > -case QEMU_MIGRATION_PHASE_FINISH3:
> > -case QEMU_MIGRATION_PHASE_PREPARE_RESUME:
> > -case QEMU_MIGRATION_PHASE_FINISH_RESUME:
> 
> What was the point of moving these to a separate section and then moving
> them back, where they were before?

No idea. Possibly wrong split between patches. I'll squash the changes
into the previous patch.

Jirka

[PATCH v2] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running

[Michal Privoznik]

When a SIGHUP is received a thread is spawned that runs
virStateReload(). However, if SIGINT is received while the former
thread is still running then we may get into problematic
situation: the cleanup code in main() sees drivers initialized
and thus calls virStateCleanup(). So now we have two threads, one
running virStateReload() the other virStateCleanup(). In this
situation it's very likely that a race condition occurs and
either of threads causes SIGSEGV.

To fix this, unmark drivers as initialized in the
virStateReload() thread for the time the function runs.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075837
Signed-off-by: Michal Privoznik 
---

v2 of:

https://listman.redhat.com/archives/libvir-list/2022-April/230415.html

diff to v1:
- reworked how int is set (instead of inc/dec I'm using set(0)/set(1))
so that reload can be attempted again and again if previous attempt
failed.

 src/remote/remote_daemon.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 26469e0d9f..b8ecc51758 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -77,7 +77,7 @@ virNetSASLContext *saslCtxt = NULL;
 virNetServerProgram *remoteProgram = NULL;
 virNetServerProgram *qemuProgram = NULL;
 
-volatile bool driversInitialized = false;
+volatile gint driversInitialized = 0;
 
 static void daemonErrorHandler(void *opaque G_GNUC_UNUSED,
virErrorPtr err G_GNUC_UNUSED)
@@ -453,8 +453,13 @@ static void daemonReloadHandlerThread(void *opaque 
G_GNUC_UNUSED)
 VIR_INFO("Reloading configuration on SIGHUP");
 virHookCall(VIR_HOOK_DRIVER_DAEMON, "-",
 VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
-if (virStateReload() < 0)
+
+if (virStateReload() < 0) {
 VIR_WARN("Error while reloading drivers");
+}
+
+/* Drivers are initialized again. */
+g_atomic_int_set(, 1);
 }
 
 static void daemonReloadHandler(virNetDaemon *dmn G_GNUC_UNUSED,
@@ -463,7 +468,7 @@ static void daemonReloadHandler(virNetDaemon *dmn 
G_GNUC_UNUSED,
 {
 virThread thr;
 
-if (!driversInitialized) {
+if (!g_atomic_int_compare_and_exchange(, 1, 0)) {
 VIR_WARN("Drivers are not initialized, reload ignored");
 return;
 }
@@ -474,6 +479,10 @@ static void daemonReloadHandler(virNetDaemon *dmn 
G_GNUC_UNUSED,
  * Not much we can do on error here except log it.
  */
 VIR_ERROR(_("Failed to create thread to handle daemon restart"));
+
+/* Drivers were initialized at the beginning, otherwise we wouldn't
+ * even get here. */
+g_atomic_int_set(, 1);
 }
 }
 
@@ -607,7 +616,7 @@ static void daemonRunStateInit(void *opaque)
 goto cleanup;
 }
 
-driversInitialized = true;
+g_atomic_int_set(, 1);
 
 virNetDaemonSetShutdownCallbacks(dmn,
  virStateShutdownPrepare,
@@ -1212,10 +1221,9 @@ int main(int argc, char **argv) {
  cleanup:
 virNetlinkEventServiceStopAll();
 
-if (driversInitialized) {
+if (g_atomic_int_compare_and_exchange(, 1, 0)) {
 /* NB: Possible issue with timing window between driversInitialized
  * setting if virNetlinkEventServerStart fails */
-driversInitialized = false;
 virStateCleanup();
 }
 
-- 
2.35.1

Re: [PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running

[Michal Prívozník]

On 5/19/22 15:27, Martin Kletzander wrote:
> On Mon, Apr 25, 2022 at 11:06:06AM +0200, Michal Privoznik wrote:
>> When a SIGHUP is received a thread is spawned that runs
>> virStateReload(). However, if SIGINT is received while the former
>> thread is still running then we may get into problematic
>> situation: the cleanup code in main() sees drivers initialized
>> and thus calls virStateCleanup(). So now we have two threads, one
>> running virStateReload() the other virStateCleanup(). In this
>> situation it's very likely that a race condition occurs and
>> either of threads causes SIGSEGV.
>>
>> To fix this, unmark drivers as initialized in the
>> virStateReload() thread for the time the function runs.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075837
>> Signed-off-by: Michal Privoznik 
>> ---
>> src/remote/remote_daemon.c | 17 +++--
>> 1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
>> index 26469e0d9f..37d27f93f4 100644
>> --- a/src/remote/remote_daemon.c
>> +++ b/src/remote/remote_daemon.c
>> @@ -77,7 +77,7 @@ virNetSASLContext *saslCtxt = NULL;
>> virNetServerProgram *remoteProgram = NULL;
>> virNetServerProgram *qemuProgram = NULL;
>>
>> -volatile bool driversInitialized = false;
>> +volatile gint driversInitialized = 0;
>>
>> static void daemonErrorHandler(void *opaque G_GNUC_UNUSED,
>>    virErrorPtr err G_GNUC_UNUSED)
>> @@ -453,8 +453,13 @@ static void daemonReloadHandlerThread(void
>> *opaque G_GNUC_UNUSED)
>>     VIR_INFO("Reloading configuration on SIGHUP");
>>     virHookCall(VIR_HOOK_DRIVER_DAEMON, "-",
>>     VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
>> -    if (virStateReload() < 0)
>> +
>> +    g_atomic_int_set(, 0);
> 
> [0] come back here after the comment below [1]
> 
> Something like the following would be a "workaround":
> 
> if (g_atomic_int_dec_and_test())
>     return;
> 
>> +    if (virStateReload() < 0) {
>>     VIR_WARN("Error while reloading drivers");
> 
> But I wonder about this.  Since we could not reload the drivers, we will
> never be able to reload them again.  I guess we leave it to the user to
> restart the daemon whenever they find a non-functioning API, right?  I
> guess that's fine, it's just that before the reload could've been made
> again and maybe increasing it to 1 again would not be that big of a deal.

Alright, I can do that.

> 
>> +    } else {
>> +    g_atomic_int_inc();
>> +    }
>> }
>>
>> static void daemonReloadHandler(virNetDaemon *dmn G_GNUC_UNUSED,
>> @@ -463,7 +468,7 @@ static void daemonReloadHandler(virNetDaemon *dmn
>> G_GNUC_UNUSED,
>> {
>>     virThread thr;
>>
>> -    if (!driversInitialized) {
>> +    if (g_atomic_int_get() == 0) {
> 
> [1] I would prefer something like g_atomic_int_inc_and_test (which does
> not exist) here so that there are no two reload handlers running at the
> same time, essentially eliminating the race window.  Since it does not
> exist we can do [0]

But g_atomic_int_add() exists and according to documentation it's atomic
version of:

  { tmp = *atomic; *atomic += val; return tmp; }

meaning if two threads execute g_atomic_int_add(, 1)
only one will see retval of 0, whilst the other will see 1.

Let me see if I can rewrite the code and work your comments in.

Michal

Re: [PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running

[Martin Kletzander]

On Mon, Apr 25, 2022 at 11:06:06AM +0200, Michal Privoznik wrote:

When a SIGHUP is received a thread is spawned that runs
virStateReload(). However, if SIGINT is received while the former
thread is still running then we may get into problematic
situation: the cleanup code in main() sees drivers initialized
and thus calls virStateCleanup(). So now we have two threads, one
running virStateReload() the other virStateCleanup(). In this
situation it's very likely that a race condition occurs and
either of threads causes SIGSEGV.

To fix this, unmark drivers as initialized in the
virStateReload() thread for the time the function runs.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075837
Signed-off-by: Michal Privoznik 
---
src/remote/remote_daemon.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 26469e0d9f..37d27f93f4 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -77,7 +77,7 @@ virNetSASLContext *saslCtxt = NULL;
virNetServerProgram *remoteProgram = NULL;
virNetServerProgram *qemuProgram = NULL;

-volatile bool driversInitialized = false;
+volatile gint driversInitialized = 0;

static void daemonErrorHandler(void *opaque G_GNUC_UNUSED,
   virErrorPtr err G_GNUC_UNUSED)
@@ -453,8 +453,13 @@ static void daemonReloadHandlerThread(void *opaque 
G_GNUC_UNUSED)
VIR_INFO("Reloading configuration on SIGHUP");
virHookCall(VIR_HOOK_DRIVER_DAEMON, "-",
VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
-if (virStateReload() < 0)
+
+g_atomic_int_set(, 0);


[0] come back here after the comment below [1]

Something like the following would be a "workaround":

if (g_atomic_int_dec_and_test())
return;


+if (virStateReload() < 0) {
VIR_WARN("Error while reloading drivers");


But I wonder about this.  Since we could not reload the drivers, we will
never be able to reload them again.  I guess we leave it to the user to
restart the daemon whenever they find a non-functioning API, right?  I
guess that's fine, it's just that before the reload could've been made
again and maybe increasing it to 1 again would not be that big of a deal.


+} else {
+g_atomic_int_inc();
+}
}

static void daemonReloadHandler(virNetDaemon *dmn G_GNUC_UNUSED,
@@ -463,7 +468,7 @@ static void daemonReloadHandler(virNetDaemon *dmn 
G_GNUC_UNUSED,
{
virThread thr;

-if (!driversInitialized) {
+if (g_atomic_int_get() == 0) {


[1] I would prefer something like g_atomic_int_inc_and_test (which does
not exist) here so that there are no two reload handlers running at the
same time, essentially eliminating the race window.  Since it does not
exist we can do [0]


VIR_WARN("Drivers are not initialized, reload ignored");
return;
}
@@ -607,7 +612,7 @@ static void daemonRunStateInit(void *opaque)
goto cleanup;
}

-driversInitialized = true;
+g_atomic_int_inc();

virNetDaemonSetShutdownCallbacks(dmn,
 virStateShutdownPrepare,
@@ -1212,10 +1217,10 @@ int main(int argc, char **argv) {
 cleanup:
virNetlinkEventServiceStopAll();

-if (driversInitialized) {
+if (g_atomic_int_get() != 0) {
/* NB: Possible issue with timing window between driversInitialized
 * setting if virNetlinkEventServerStart fails */
-driversInitialized = false;
+g_atomic_int_set(, 0);
virStateCleanup();
}

--
2.35.1



signature.asc
Description: PGP signature

Re: [libvirt PATCH 59/80] qemu: Implement VIR_MIGRATE_POSTCOPY_RESUME for Confirm phase

[Jiri Denemark]

On Thu, May 12, 2022 at 16:27:24 +0200, Peter Krempa wrote:
> On Tue, May 10, 2022 at 17:21:20 +0200, Jiri Denemark wrote:
> > Signed-off-by: Jiri Denemark 
> > ---
> >  src/qemu/qemu_migration.c | 49 +--
> >  1 file changed, 32 insertions(+), 17 deletions(-)
> 
> [...]
> 
> >  
> >  if (qemuMigrationJobStartPhase(vm, phase) < 0)
> >  goto cleanup;
> >  
> >  virCloseCallbacksUnset(driver->closeCallbacks, vm,
> > qemuMigrationSrcCleanup);
> > +qemuDomainCleanupRemove(vm, qemuProcessCleanupMigrationJob);
> 
> This looks out of place in context of this commit.

Sigh. I was trying hard to remember the reason I put this change into
this patch because there must have been one... but it was just a mistake
and the change belongs to [14/80] qemu: Keep migration job active after
failed post-copy.

Jirka

Re: [PATCH] Allow VM to read sysfs PCI config, revision files

[Michal Prívozník]

On 5/19/22 08:09, Christian Ehrhardt wrote:
> On Thu, May 12, 2022 at 3:27 PM Max Goodhart  wrote:
>>
>> Oops, I didn't intend for the commit author email to be 
>> git...@chromakode.com here. Would you please use c...@chromakode.com as the 
>> author of the patch?
> 
> Yeah I can amend that (and we see you control that email address).
> 
> Also since I know this is coming from a particular LP bug I'd also add
> the following to have the full trace-back from the commit message to
> more context
> 
> Fixes: https://launchpad.net/bugs/1972075
> 
> Can I apply it that way or are there any objections?

Yeah, go ahead and push. The change does indeed fix the problem.

Michal

Re: [libvirt PATCH 2/5] src: QemuMonitorCommandWithFiles: report error when fd passing is unsupported

[Martin Kletzander]

On Wed, May 18, 2022 at 04:00:57PM +0200, Ján Tomko wrote:

The result of the <= 0 comparison was assigned to 'rc', rendering the
if (rc == 0) condition dead code.

Signed-off-by: Ján Tomko 
---
src/libvirt-qemu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c
index 5020b5dc1b..ace91e8ada 100644
--- a/src/libvirt-qemu.c
+++ b/src/libvirt-qemu.c
@@ -161,7 +161,7 @@ virDomainQemuMonitorCommandWithFiles(virDomainPtr domain,
if (ninfiles > 0 || outfiles) {
int rc;
if ((rc = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn,
-   VIR_DRV_FEATURE_FD_PASSING) <= 0)) {
+   VIR_DRV_FEATURE_FD_PASSING)) <= 0) {
if (rc == 0)
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
   _("fd passing is not supported by this 
connection"));
--
2.34.1



Or even (way) better:

diff --git i/src/libvirt-qemu.c w/src/libvirt-qemu.c
index 3cd8c8f7453d..93107d1bfcbb 100644
--- i/src/libvirt-qemu.c
+++ w/src/libvirt-qemu.c
@@ -159,9 +159,9 @@ virDomainQemuMonitorCommandWithFiles(virDomainPtr domain,
 virCheckNonNullArgGoto(cmd, error);

 if (ninfiles > 0 || outfiles) {
-int rc;
-if ((rc = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn,
-   VIR_DRV_FEATURE_FD_PASSING) <= 0)) {
+int rc = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn,
+  VIR_DRV_FEATURE_FD_PASSING);
+if (rc <= 0) {
 if (rc == 0)
 virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
_("fd passing is not supported by this 
connection"));
--


signature.asc
Description: PGP signature

[PATCH] apparmor: Add support for dbus chardev

[Martin Kletzander]

Commit 7648e40da50e added support for dbus chardev but forgot to handle it in
AppArmor code.

Signed-off-by: Martin Kletzander 
---
Pushed under the build-breaker rule.

 src/security/security_apparmor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 55c019394050..008384dee8c2 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -1022,6 +1022,7 @@ AppArmorSetChardevLabel(virSecurityManager *mgr,
 case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
 case VIR_DOMAIN_CHR_TYPE_NMDM:
 case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT:
+case VIR_DOMAIN_CHR_TYPE_DBUS:
 case VIR_DOMAIN_CHR_TYPE_LAST:
 ret = 0;
 break;
@@ -1085,6 +1086,7 @@ AppArmorSetNetdevLabel(virSecurityManager *mgr,
 case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
 case VIR_DOMAIN_CHR_TYPE_NMDM:
 case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT:
+case VIR_DOMAIN_CHR_TYPE_DBUS:
 case VIR_DOMAIN_CHR_TYPE_LAST:
 ret = 0;
 break;
-- 
2.35.1

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Gerd Hoffmann]

  Hi,

> Hmm, ok ... but maybe I should call the new enum HotKeyMod instead of
> GrabMod to make it more obvious that it is something different?

Looks good to me.

take care,
  Gerd

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Thomas Huth]

On 19/05/2022 12.48, Gerd Hoffmann wrote:

   Hi,


Oh well, I just noticed that we already have a GrabToggleKeys enum in
qapi/common.json ... I wonder whether I should try to use that instead? It
seems to be used in a slightly different context, though, if I get that
right ...?


qemu/ui/input-linux.c

Those switch the input routing between host and guest, and they act on
their own (i.e. by default both control keys without anything else).

For SDL it defines the modifiers to press in addition to the hotkeys
(i.e. ctrl + shift + 'G' for grab release, IIRC there a are more, 'F'
for fullscreen?).

So I don't think it makes sense to merge them.


Hmm, ok ... but maybe I should call the new enum HotKeyMod instead of 
GrabMod to make it more obvious that it is something different?


 Thomas

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Gerd Hoffmann]

  Hi,

> Oh well, I just noticed that we already have a GrabToggleKeys enum in
> qapi/common.json ... I wonder whether I should try to use that instead? It
> seems to be used in a slightly different context, though, if I get that
> right ...?

qemu/ui/input-linux.c

Those switch the input routing between host and guest, and they act on
their own (i.e. by default both control keys without anything else).

For SDL it defines the modifiers to press in addition to the hotkeys
(i.e. ctrl + shift + 'G' for grab release, IIRC there a are more, 'F'
for fullscreen?).

So I don't think it makes sense to merge them.

take care,
  Gerd

Re: [libvirt PATCH v4 00/11] Add QEMU "-display dbus" support

[Michal Prívozník]

On 5/13/22 20:38, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> Hi,
> 
> This series implements supports for the QEMU "-display dbus" support, 
> available
> since 7.0.
> 
> By default, libvirt will start a private VM bus (sharing and reusing the
> existing "vmstate" VM bus & code).
> 
> The feature set should cover the needs to replace Spice as local client of 
> choice,
> including 3daccel/dmabuf, audio, clipboard sharing, usb redirection, and 
> arbitrary
> chardev/channels (for serial etc).
> 
> The test Gtk4 client is also in progress, currently in development at
> https://gitlab.com/marcandre.lureau/qemu-display/. virt-viewer & boxes will 
> need
> a port to Gtk4 to make use of the shared widget.
> 
> thanks
> 
> v4:
>  - rebased, dropped some patches, updated versions in docs, code adjustments 
> etc
>  - dropped clipboard option, as standalone qemu-vdagent support landed
> 
> v3: after QEMU 7.0 dev cycle opening and merge
>  - rebased
>  - add 7.0 x86-64 capabilities (instead of tweaking 6.2)
>  - fix version annotations
> 
> Marc-André Lureau (11):
>   qemu: add -display dbus capability check
>   conf: add 
>   qemu: start the D-Bus daemon for the display
>   qemu: add -display dbus support
>   virsh: report the D-Bus bus URI for domdisplay
>   conf: add  support
>   qemu: add audio type 'dbus'
>   conf: add 
>   qemu: add -chardev dbus support
>   qemu: add usbredir type 'dbus'
>   docs: document  type dbus
> 

>  52 files changed, 948 insertions(+), 20 deletions(-)

> 

Reviewed-by: Michal Privoznik 

and pushed.

Michal

Plans for the next release

[Jiri Denemark]

We are getting close to the next release of libvirt. To aim for the
release on June 01 I suggest entering the freeze on Wednesday May 25 in
the evening and tagging RC2 on Monday May 30.

I hope this works for everyone.

Jirka

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Thomas Huth]

On 19/05/2022 09.51, Daniel P. Berrangé wrote:

On Thu, May 19, 2022 at 09:27:08AM +0200, Thomas Huth wrote:

On 19/05/2022 09.08, Thomas Huth wrote:

On 19/05/2022 08.39, Thomas Huth wrote:

On 18/05/2022 17.08, Markus Armbruster wrote:

Thomas Huth  writes:


The "-display sdl" option still uses a hand-crafted parser for its
parameters since we didn't want to drag an interface we considered
somewhat flawed into the QAPI schema. Since the flaws are gone now,
it's time to QAPIfy.

This introduces the new "DisplaySDL" QAPI struct that is used to hold
the parameters that are unique to the SDL display. The only specific
parameter is currently "grab-mod" that is used to specify the required
modifier keys to escape from the mouse grabbing mode.

Signed-off-by: Thomas Huth 
---
   qapi/ui.json    | 27 +++-
   include/sysemu/sysemu.h |  2 --
   softmmu/globals.c   |  2 --
   softmmu/vl.c    | 70 +
   ui/sdl2.c   | 10 ++
   5 files changed, 37 insertions(+), 74 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 11a827d10f..a244e26e0f 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1295,6 +1295,30 @@
     '*swap-opt-cmd': 'bool'
     } }
+##
+# @GrabMod:
+#
+# Set of modifier keys that need to be hold for shortcut key actions.
+#
+# Since: 7.1
+##
+{ 'enum'  : 'GrabMod',
+  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }


This is fine now.  If we ever generalize to "arbitrary set of modifier
keys", it'll become somewhat awkward.  No objection from me.


Oh well, I just noticed that we already have a GrabToggleKeys enum in
qapi/common.json ... I wonder whether I should try to use that instead? It
seems to be used in a slightly different context, though, if I get that
right ...?


It also doesn't distinguish left & right control/alt/shift keys
for some reason.  So you would end up having to add more enum
entries for SDL, none of which overlap with existing enum entries.


We could also extend the SDL code to work with the other combos from 
GrabToggleKeys, I guess.



Rather a pity, as the consistency would have been nice


I wonder which way would cause less "WTF?" situations in the future ... if 
we have one enum with slightly different naming between the entries, or if 
we have two enums that seems to be there for the same or at least very 
similar things, which still have still inconsistent namings between the 
entries...

I'm slightly inclined to go for the unified GrabToggleKeys enum, I think...

 Thomas

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Thomas Huth]

On 19/05/2022 10.57, Markus Armbruster wrote:

Daniel P. Berrangé  writes:


On Thu, May 19, 2022 at 09:27:08AM +0200, Thomas Huth wrote:

On 19/05/2022 09.08, Thomas Huth wrote:

On 19/05/2022 08.39, Thomas Huth wrote:

On 18/05/2022 17.08, Markus Armbruster wrote:

Thomas Huth  writes:


The "-display sdl" option still uses a hand-crafted parser for its
parameters since we didn't want to drag an interface we considered
somewhat flawed into the QAPI schema. Since the flaws are gone now,
it's time to QAPIfy.

This introduces the new "DisplaySDL" QAPI struct that is used to hold
the parameters that are unique to the SDL display. The only specific
parameter is currently "grab-mod" that is used to specify the required
modifier keys to escape from the mouse grabbing mode.

Signed-off-by: Thomas Huth 
---
   qapi/ui.json    | 27 +++-
   include/sysemu/sysemu.h |  2 --
   softmmu/globals.c   |  2 --
   softmmu/vl.c    | 70 +
   ui/sdl2.c   | 10 ++
   5 files changed, 37 insertions(+), 74 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 11a827d10f..a244e26e0f 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1295,6 +1295,30 @@
     '*swap-opt-cmd': 'bool'
     } }
+##
+# @GrabMod:
+#
+# Set of modifier keys that need to be hold for shortcut key actions.
+#
+# Since: 7.1
+##
+{ 'enum'  : 'GrabMod',
+  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }


This is fine now.  If we ever generalize to "arbitrary set of modifier
keys", it'll become somewhat awkward.  No objection from me.


Oh well, I just noticed that we already have a GrabToggleKeys enum in
qapi/common.json ... I wonder whether I should try to use that instead? It
seems to be used in a slightly different context, though, if I get that
right ...?


It also doesn't distinguish left & right control/alt/shift keys
for some reason.  So you would end up having to add more enum
entries for SDL, none of which overlap with existing enum entries.
Rather a pity, as the consistency would have been nice


Speaking of consistency: stick to the key names we use in QKeyCode?
Sadly, they contain '_'.


The "grab-mod" with the current name already exists, so if we want to switch 
to those names, we need to deprecate the current ones again ... and the 
enums would really look ugly in the code, too, e.g. CTRL_L_ALT_L ... hard to 
tell where the "L" really belongs to ... so I'd rather stick with the 
current names, I think.


 Thomas

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Thu, May 19, 2022 at 09:27:08AM +0200, Thomas Huth wrote:
>> On 19/05/2022 09.08, Thomas Huth wrote:
>> > On 19/05/2022 08.39, Thomas Huth wrote:
>> > > On 18/05/2022 17.08, Markus Armbruster wrote:
>> > > > Thomas Huth  writes:
>> > > > 
>> > > > > The "-display sdl" option still uses a hand-crafted parser for its
>> > > > > parameters since we didn't want to drag an interface we considered
>> > > > > somewhat flawed into the QAPI schema. Since the flaws are gone now,
>> > > > > it's time to QAPIfy.
>> > > > > 
>> > > > > This introduces the new "DisplaySDL" QAPI struct that is used to hold
>> > > > > the parameters that are unique to the SDL display. The only specific
>> > > > > parameter is currently "grab-mod" that is used to specify the 
>> > > > > required
>> > > > > modifier keys to escape from the mouse grabbing mode.
>> > > > > 
>> > > > > Signed-off-by: Thomas Huth 
>> > > > > ---
>> > > > >   qapi/ui.json    | 27 +++-
>> > > > >   include/sysemu/sysemu.h |  2 --
>> > > > >   softmmu/globals.c   |  2 --
>> > > > >   softmmu/vl.c    | 70 
>> > > > > +
>> > > > >   ui/sdl2.c   | 10 ++
>> > > > >   5 files changed, 37 insertions(+), 74 deletions(-)
>> > > > > 
>> > > > > diff --git a/qapi/ui.json b/qapi/ui.json
>> > > > > index 11a827d10f..a244e26e0f 100644
>> > > > > --- a/qapi/ui.json
>> > > > > +++ b/qapi/ui.json
>> > > > > @@ -1295,6 +1295,30 @@
>> > > > >     '*swap-opt-cmd': 'bool'
>> > > > >     } }
>> > > > > +##
>> > > > > +# @GrabMod:
>> > > > > +#
>> > > > > +# Set of modifier keys that need to be hold for shortcut key 
>> > > > > actions.
>> > > > > +#
>> > > > > +# Since: 7.1
>> > > > > +##
>> > > > > +{ 'enum'  : 'GrabMod',
>> > > > > +  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }
>> > > > 
>> > > > This is fine now.  If we ever generalize to "arbitrary set of modifier
>> > > > keys", it'll become somewhat awkward.  No objection from me.
>> 
>> Oh well, I just noticed that we already have a GrabToggleKeys enum in
>> qapi/common.json ... I wonder whether I should try to use that instead? It
>> seems to be used in a slightly different context, though, if I get that
>> right ...?
>
> It also doesn't distinguish left & right control/alt/shift keys
> for some reason.  So you would end up having to add more enum
> entries for SDL, none of which overlap with existing enum entries.
> Rather a pity, as the consistency would have been nice

Speaking of consistency: stick to the key names we use in QKeyCode?
Sadly, they contain '_'.

Re: [PATCH 0/5] Some docs updates

[Andrea Bolognani]

On Sat, May 07, 2022 at 09:17:35AM +0800, Han Han wrote:
> Han Han (5):
>   docs: apps: Add desktop app gnome-boxes
>   docs: formatdomain: Add the introduced versions of net rss attrs
>   news: Add news for rss and rss_hash_report attributes
>   docs: drivers: Mention KVM/HVF in the link of qemu driver
>   docs: drvqemu: Fix the syntax typo of Hypervisor.framework link

Series

  Reviewed-by: Andrea Bolognani 

and pushed. Thanks!

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 1/5] docs: apps: Add desktop app gnome-boxes

[Andrea Bolognani]

On Sat, May 07, 2022 at 09:17:36AM +0800, Han Han wrote:
> +`gnome-boxes `__
> +   A gnome application to access virtual machines.

The correct URL according to [1] is [2] (which currently redirects to
[3]). I've changed that as well as spelling GNOME and GNOME Boxes the
preferred way.


[1] https://gitlab.gnome.org/GNOME/gnome-boxes
[2] https://gnomeboxes.org/
[3] https://apps.gnome.org/app/org.gnome.Boxes/
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] remoteOpenConn: Pass correct variable to virConnectSetIdentity()

[Erik Skultety]

On Thu, May 19, 2022 at 10:00:26AM +0200, Michal Privoznik wrote:
> The remoteOpenConn() function was refactored recently. As a part
> of that new variable @newconn was introduced which holds
> virConnect object as it's being gradually constructed throughout
> the function. At the very end, when everything succeeded the
> variable is stolen into passed @conn. However, there was one
> line missed in the refactor which still access the @conn instead
> of @newconn leading to a NULL dereference.
> 
> Fixes: f7c422993e4c7ca3e58b1d0d69f4772851af399f
> Signed-off-by: Michal Privoznik 
> ---

CI++

Reviewed-by: Erik Skultety 

[PATCH] remoteOpenConn: Pass correct variable to virConnectSetIdentity()

[Michal Privoznik]

The remoteOpenConn() function was refactored recently. As a part
of that new variable @newconn was introduced which holds
virConnect object as it's being gradually constructed throughout
the function. At the very end, when everything succeeded the
variable is stolen into passed @conn. However, there was one
line missed in the refactor which still access the @conn instead
of @newconn leading to a NULL dereference.

Fixes: f7c422993e4c7ca3e58b1d0d69f4772851af399f
Signed-off-by: Michal Privoznik 
---
 src/remote/remote_daemon_dispatch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/remote/remote_daemon_dispatch.c 
b/src/remote/remote_daemon_dispatch.c
index c1f85925a3..0fde4233a5 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1829,7 +1829,7 @@ remoteOpenConn(const char *uri,
 VIR_DEBUG("Opened driver %p", newconn);
 
 if (preserveIdentity) {
-if (virConnectSetIdentity(*conn, identparams->par, identparams->npar, 
0) < 0)
+if (virConnectSetIdentity(newconn, identparams->par, 
identparams->npar, 0) < 0)
 return -1;
 
 VIR_DEBUG("Forwarded current identity to secondary driver");
-- 
2.35.1

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Daniel P . Berrangé]

On Thu, May 19, 2022 at 09:27:08AM +0200, Thomas Huth wrote:
> On 19/05/2022 09.08, Thomas Huth wrote:
> > On 19/05/2022 08.39, Thomas Huth wrote:
> > > On 18/05/2022 17.08, Markus Armbruster wrote:
> > > > Thomas Huth  writes:
> > > > 
> > > > > The "-display sdl" option still uses a hand-crafted parser for its
> > > > > parameters since we didn't want to drag an interface we considered
> > > > > somewhat flawed into the QAPI schema. Since the flaws are gone now,
> > > > > it's time to QAPIfy.
> > > > > 
> > > > > This introduces the new "DisplaySDL" QAPI struct that is used to hold
> > > > > the parameters that are unique to the SDL display. The only specific
> > > > > parameter is currently "grab-mod" that is used to specify the required
> > > > > modifier keys to escape from the mouse grabbing mode.
> > > > > 
> > > > > Signed-off-by: Thomas Huth 
> > > > > ---
> > > > >   qapi/ui.json    | 27 +++-
> > > > >   include/sysemu/sysemu.h |  2 --
> > > > >   softmmu/globals.c   |  2 --
> > > > >   softmmu/vl.c    | 70 
> > > > > +
> > > > >   ui/sdl2.c   | 10 ++
> > > > >   5 files changed, 37 insertions(+), 74 deletions(-)
> > > > > 
> > > > > diff --git a/qapi/ui.json b/qapi/ui.json
> > > > > index 11a827d10f..a244e26e0f 100644
> > > > > --- a/qapi/ui.json
> > > > > +++ b/qapi/ui.json
> > > > > @@ -1295,6 +1295,30 @@
> > > > >     '*swap-opt-cmd': 'bool'
> > > > >     } }
> > > > > +##
> > > > > +# @GrabMod:
> > > > > +#
> > > > > +# Set of modifier keys that need to be hold for shortcut key actions.
> > > > > +#
> > > > > +# Since: 7.1
> > > > > +##
> > > > > +{ 'enum'  : 'GrabMod',
> > > > > +  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }
> > > > 
> > > > This is fine now.  If we ever generalize to "arbitrary set of modifier
> > > > keys", it'll become somewhat awkward.  No objection from me.
> 
> Oh well, I just noticed that we already have a GrabToggleKeys enum in
> qapi/common.json ... I wonder whether I should try to use that instead? It
> seems to be used in a slightly different context, though, if I get that
> right ...?

It also doesn't distinguish left & right control/alt/shift keys
for some reason.  So you would end up having to add more enum
entries for SDL, none of which overlap with existing enum entries.
Rather a pity, as the consistency would have been nice

With 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 :|

Re: [PATCH v2 2/2] cpu_ppc64: add support for host-model on POWER10

[Andrea Bolognani]

On Tue, May 17, 2022 at 05:32:56PM -0300, Daniel Henrique Barboza wrote:
> On 5/12/22 04:52, Andrea Bolognani wrote:
> > Don't forget to add the new test case to qemuxml2xmltest too.
> >
> > It would be great if, as a follow-up, you could look into converting
> > these test cases to the DO_TEST_CAPS_ARCH_LATEST_* macros, which
> > would allow us to drop the hardcoded list of capabilities.
>
> Right. Guess I'll use ARCH_LATEST in the tests I'm adding in this patch as 
> well.

Yeah, that'd be fantastic :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Markus Armbruster]

Thomas Huth  writes:

> On 18/05/2022 17.08, Markus Armbruster wrote:
>> Thomas Huth  writes:
>> 
>>> The "-display sdl" option still uses a hand-crafted parser for its
>>> parameters since we didn't want to drag an interface we considered
>>> somewhat flawed into the QAPI schema. Since the flaws are gone now,
>>> it's time to QAPIfy.
>>>
>>> This introduces the new "DisplaySDL" QAPI struct that is used to hold
>>> the parameters that are unique to the SDL display. The only specific
>>> parameter is currently "grab-mod" that is used to specify the required
>>> modifier keys to escape from the mouse grabbing mode.
>>>
>>> Signed-off-by: Thomas Huth 
>>> ---
>>>   qapi/ui.json| 27 +++-
>>>   include/sysemu/sysemu.h |  2 --
>>>   softmmu/globals.c   |  2 --
>>>   softmmu/vl.c| 70 +
>>>   ui/sdl2.c   | 10 ++
>>>   5 files changed, 37 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/qapi/ui.json b/qapi/ui.json
>>> index 11a827d10f..a244e26e0f 100644
>>> --- a/qapi/ui.json
>>> +++ b/qapi/ui.json
>>> @@ -1295,6 +1295,30 @@
>>> '*swap-opt-cmd': 'bool'
>>> } }
>>>   
>>> +##
>>> +# @GrabMod:
>>> +#
>>> +# Set of modifier keys that need to be hold for shortcut key actions.
>>> +#
>>> +# Since: 7.1
>>> +##
>>> +{ 'enum'  : 'GrabMod',
>>> +  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }
>> 
>> This is fine now.  If we ever generalize to "arbitrary set of modifier
>> keys", it'll become somewhat awkward.  No objection from me.
>> 
>>> +
>>> +##
>>> +# @DisplaySDL:
>>> +#
>>> +# SDL2 display options.
>>> +#
>>> +# @grab-mod:  String with modifier keys that should be pressed together 
>>> with
>> 
>> s/String with modifier keys/Modifier keys/
>> 
>>> +# the "G" key to release the mouse grab. Only 
>>> "lshift-lctrl-lalt"
>>> +# and "rctrl" are currently supported.
>> 
>> Why do we define lctrl-lalt if it's not supported?
>
> It's the default value if you don't specify the grab-mod parameter. So it's 
> supported, you get this mode if you use "grab-mod=lctrl-lalt" or no grab-mod 
> parameter at all.

That's not how I understood the comment...

Suggest

# @grab-mod: Modifier keys that should be pressed together with the "G"
#key to release the mouse grab (default "lctrl-lalt")

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Thomas Huth]

On 19/05/2022 09.08, Thomas Huth wrote:

On 19/05/2022 08.39, Thomas Huth wrote:

On 18/05/2022 17.08, Markus Armbruster wrote:

Thomas Huth  writes:


The "-display sdl" option still uses a hand-crafted parser for its
parameters since we didn't want to drag an interface we considered
somewhat flawed into the QAPI schema. Since the flaws are gone now,
it's time to QAPIfy.

This introduces the new "DisplaySDL" QAPI struct that is used to hold
the parameters that are unique to the SDL display. The only specific
parameter is currently "grab-mod" that is used to specify the required
modifier keys to escape from the mouse grabbing mode.

Signed-off-by: Thomas Huth 
---
  qapi/ui.json    | 27 +++-
  include/sysemu/sysemu.h |  2 --
  softmmu/globals.c   |  2 --
  softmmu/vl.c    | 70 +
  ui/sdl2.c   | 10 ++
  5 files changed, 37 insertions(+), 74 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 11a827d10f..a244e26e0f 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1295,6 +1295,30 @@
    '*swap-opt-cmd': 'bool'
    } }
+##
+# @GrabMod:
+#
+# Set of modifier keys that need to be hold for shortcut key actions.
+#
+# Since: 7.1
+##
+{ 'enum'  : 'GrabMod',
+  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }


This is fine now.  If we ever generalize to "arbitrary set of modifier
keys", it'll become somewhat awkward.  No objection from me.


Oh well, I just noticed that we already have a GrabToggleKeys enum in 
qapi/common.json ... I wonder whether I should try to use that instead? It 
seems to be used in a slightly different context, though, if I get that 
right ...?


 Thomas

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Thomas Huth]

On 19/05/2022 08.39, Thomas Huth wrote:

On 18/05/2022 17.08, Markus Armbruster wrote:

Thomas Huth  writes:


The "-display sdl" option still uses a hand-crafted parser for its
parameters since we didn't want to drag an interface we considered
somewhat flawed into the QAPI schema. Since the flaws are gone now,
it's time to QAPIfy.

This introduces the new "DisplaySDL" QAPI struct that is used to hold
the parameters that are unique to the SDL display. The only specific
parameter is currently "grab-mod" that is used to specify the required
modifier keys to escape from the mouse grabbing mode.

Signed-off-by: Thomas Huth 
---
  qapi/ui.json    | 27 +++-
  include/sysemu/sysemu.h |  2 --
  softmmu/globals.c   |  2 --
  softmmu/vl.c    | 70 +
  ui/sdl2.c   | 10 ++
  5 files changed, 37 insertions(+), 74 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 11a827d10f..a244e26e0f 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1295,6 +1295,30 @@
    '*swap-opt-cmd': 'bool'
    } }
+##
+# @GrabMod:
+#
+# Set of modifier keys that need to be hold for shortcut key actions.
+#
+# Since: 7.1
+##
+{ 'enum'  : 'GrabMod',
+  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }


This is fine now.  If we ever generalize to "arbitrary set of modifier
keys", it'll become somewhat awkward.  No objection from me.


+
+##
+# @DisplaySDL:
+#
+# SDL2 display options.
+#
+# @grab-mod:  String with modifier keys that should be pressed together 
with


s/String with modifier keys/Modifier keys/

+# the "G" key to release the mouse grab. Only 
"lshift-lctrl-lalt"

+# and "rctrl" are currently supported.


Why do we define lctrl-lalt if it's not supported?


It's the default value if you don't specify the grab-mod parameter. So it's 
supported, you get this mode if you use "grab-mod=lctrl-lalt" or no grab-mod 
parameter at all.


Forgot to mention: I'll drop that sentence in v3. Since there is the enum 
now, people can look up the valid combinations there instead.


 Thomas

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Thomas Huth]

On 18/05/2022 17.41, Eric Blake wrote:

On Wed, May 18, 2022 at 03:44:45PM +0200, Thomas Huth wrote:

The "-display sdl" option still uses a hand-crafted parser for its
parameters since we didn't want to drag an interface we considered
somewhat flawed into the QAPI schema. Since the flaws are gone now,
it's time to QAPIfy.

This introduces the new "DisplaySDL" QAPI struct that is used to hold
the parameters that are unique to the SDL display. The only specific
parameter is currently "grab-mod" that is used to specify the required
modifier keys to escape from the mouse grabbing mode.

Signed-off-by: Thomas Huth 
---



+++ b/qapi/ui.json
@@ -1295,6 +1295,30 @@
'*swap-opt-cmd': 'bool'
} }
  
+##

+# @GrabMod:
+#
+# Set of modifier keys that need to be hold for shortcut key actions.


s/hold/held/


Thanks, I'll send a v3 with this (and the nits found by Markus) fixed.

 Thomas

Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Thomas Huth]

On 18/05/2022 17.08, Markus Armbruster wrote:

Thomas Huth  writes:


The "-display sdl" option still uses a hand-crafted parser for its
parameters since we didn't want to drag an interface we considered
somewhat flawed into the QAPI schema. Since the flaws are gone now,
it's time to QAPIfy.

This introduces the new "DisplaySDL" QAPI struct that is used to hold
the parameters that are unique to the SDL display. The only specific
parameter is currently "grab-mod" that is used to specify the required
modifier keys to escape from the mouse grabbing mode.

Signed-off-by: Thomas Huth 
---
  qapi/ui.json| 27 +++-
  include/sysemu/sysemu.h |  2 --
  softmmu/globals.c   |  2 --
  softmmu/vl.c| 70 +
  ui/sdl2.c   | 10 ++
  5 files changed, 37 insertions(+), 74 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 11a827d10f..a244e26e0f 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1295,6 +1295,30 @@
'*swap-opt-cmd': 'bool'
} }
  
+##

+# @GrabMod:
+#
+# Set of modifier keys that need to be hold for shortcut key actions.
+#
+# Since: 7.1
+##
+{ 'enum'  : 'GrabMod',
+  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }


This is fine now.  If we ever generalize to "arbitrary set of modifier
keys", it'll become somewhat awkward.  No objection from me.


+
+##
+# @DisplaySDL:
+#
+# SDL2 display options.
+#
+# @grab-mod:  String with modifier keys that should be pressed together with


s/String with modifier keys/Modifier keys/


+# the "G" key to release the mouse grab. Only "lshift-lctrl-lalt"
+# and "rctrl" are currently supported.


Why do we define lctrl-lalt if it's not supported?


It's the default value if you don't specify the grab-mod parameter. So it's 
supported, you get this mode if you use "grab-mod=lctrl-lalt" or no grab-mod 
parameter at all.


 Thomas

Re: [PATCH] Allow VM to read sysfs PCI config, revision files

[Christian Ehrhardt]

On Thu, May 12, 2022 at 3:27 PM Max Goodhart  wrote:
>
> Oops, I didn't intend for the commit author email to be git...@chromakode.com 
> here. Would you please use c...@chromakode.com as the author of the patch?

Yeah I can amend that (and we see you control that email address).

Also since I know this is coming from a particular LP bug I'd also add
the following to have the full trace-back from the commit message to
more context

Fixes: https://launchpad.net/bugs/1972075

Can I apply it that way or are there any objections?

> On Wed, May 11, 2022, 6:09 PM Max Goodhart  wrote:
>>
>> From: Max Goodhart 
>>
>> This fixes a blank screen when viewing a VM with virtio graphics and
>> gl-accelerated Spice display on Ubuntu 22.04 / libvirt 8.0.0 / qemu 6.2.
>>
>> Without these AppArmor permissions, the libvirt error log contains
>> repetitions of:
>>
>> qemu_spice_gl_scanout_texture: failed to get fd for texture
>>
>> This appears to be similar to this GNOME Boxes issue:
>> https://gitlab.gnome.org/GNOME/gnome-boxes/-/issues/586
>>
>> Signed-off-by: Max Goodhart 
>> ---
>>  src/security/virt-aa-helper.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>> index 1f1cce8b3d..b314d2a059 100644
>> --- a/src/security/virt-aa-helper.c
>> +++ b/src/security/virt-aa-helper.c
>> @@ -1316,7 +1316,7 @@ get_files(vahControl * ctl)
>>  virBufferAddLit(, "  \"/dev/nvidiactl\" rw,\n");
>>  virBufferAddLit(, "  # Probe DRI device attributes\n");
>>  virBufferAddLit(, "  \"/dev/dri/\" r,\n");
>> -virBufferAddLit(, "  
>> \"/sys/devices/**/{uevent,vendor,device,subsystem_vendor,subsystem_device}\" 
>> r,\n");
>> +virBufferAddLit(, "  
>> \"/sys/devices/**/{uevent,vendor,device,subsystem_vendor,subsystem_device,config,revision}\"
>>  r,\n");
>>  virBufferAddLit(, "  # dri libs will trigger that, but t is not 
>> requited and DAC would deny it anyway\n");
>>  virBufferAddLit(, "  deny \"/var/lib/libvirt/.cache/\" w,\n");
>>  }
>> --
>> 2.34.1
>>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] Allow VM to read sysfs PCI config, revision files

[Christian Ehrhardt]

On Thu, May 12, 2022 at 3:27 PM Max Goodhart  wrote:
>
> From: Max Goodhart 

Hi Max,
thanks for the work to identify and fix this!

It is indeed a natural evolution of my 27a9ebf2818 00fbb9e5167
f2cbb94eabd that made the rules so far.

Signed-off-by: Christian Ehrhardt 

> This fixes a blank screen when viewing a VM with virtio graphics and
> gl-accelerated Spice display on Ubuntu 22.04 / libvirt 8.0.0 / qemu 6.2.
>
> Without these AppArmor permissions, the libvirt error log contains
> repetitions of:
>
> qemu_spice_gl_scanout_texture: failed to get fd for texture
>
> This appears to be similar to this GNOME Boxes issue:
> https://gitlab.gnome.org/GNOME/gnome-boxes/-/issues/586
>
> Signed-off-by: Max Goodhart 
> ---
>  src/security/virt-aa-helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 1f1cce8b3d..b314d2a059 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1316,7 +1316,7 @@ get_files(vahControl * ctl)
>  virBufferAddLit(, "  \"/dev/nvidiactl\" rw,\n");
>  virBufferAddLit(, "  # Probe DRI device attributes\n");
>  virBufferAddLit(, "  \"/dev/dri/\" r,\n");
> -virBufferAddLit(, "  
> \"/sys/devices/**/{uevent,vendor,device,subsystem_vendor,subsystem_device}\" 
> r,\n");
> +virBufferAddLit(, "  
> \"/sys/devices/**/{uevent,vendor,device,subsystem_vendor,subsystem_device,config,revision}\"
>  r,\n");
>  virBufferAddLit(, "  # dri libs will trigger that, but t is not 
> requited and DAC would deny it anyway\n");
>  virBufferAddLit(, "  deny \"/var/lib/libvirt/.cache/\" w,\n");
>  }
> --
> 2.34.1
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd