Re: [PATCH 0/2] hw/core: revert deprecation of 'parameter=1' for SMP topology

[Ján Tomko]

On a Monday in 2024, Daniel P. Berrangé wrote:

Since QEMU 9.0, users are complaining that depecation messages are shown
for every VM libvirt starts. This is due to the newly introduced
deprecation of 'parameter=1' for -smp. This proposes reverting that, see
the 1st patch for further commentary.

Daniel P. Berrangé (2):
 hw/core: allow parameter=1 for CPU topology on any machine
 tests: add testing of parameter=1 for SMP topology

docs/about/deprecated.rst   | 14 ---
hw/core/machine-smp.c   | 82 -
tests/unit/test-smp-parse.c | 16 ++--
3 files changed, 38 insertions(+), 74 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 4/4] qapi/char: Deprecate backend type "memory"

[Ján Tomko]

On a Saturday in 2024, Markus Armbruster wrote:

It's an alias for "ringbuf" we kept for backward compatibility; see
commit 3a1da42eb35 (qapi: Rename ChardevBackend member "memory" to
"ringbuf").  Deprecation is long overdue.

Signed-off-by: Markus Armbruster 
---
docs/about/deprecated.rst | 8 
qapi/char.json| 8 +---
2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index d4492b9460..ae1a520c26 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -371,6 +371,14 @@ Specifying the iSCSI password in plain text on the command 
line using the
used instead, to refer to a ``--object secret...`` instance that provides
a password via a file, or encrypted.

+Character device options
+
+
+Backend ``memory`` (since 9.0)
+^^
+
+``memory`` is a deprecated synonym for ``ringbuf``.
+


For libvirt:
Reviewed-by: Ján Tomko 

(We don't support either of those)

Jano


signature.asc
Description: PGP signature

Re: [PATCH] os-posix: Allow 'chroot' via '-run-with' and deprecate the old '-chroot' option

[Ján Tomko]

On a Friday in 2023, Thomas Huth wrote:

We recently introduced "-run-with" for options that influence the
runtime behavior of QEMU. This option has the big advantage that it
can group related options (so that it is easier for the users to spot
them) and that the options become introspectable via QMP this way.
So let's start moving more switches into this option group, starting
with "-chroot" now.

Signed-off-by: Thomas Huth 
---
docs/about/deprecated.rst |  5 +
os-posix.c| 35 ++-
util/async-teardown.c | 21 -
qemu-options.hx   | 18 +-
4 files changed, 52 insertions(+), 27 deletions(-)



For libvirt:

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: query-command-line-options

[Ján Tomko]

On a Friday in 2023, Markus Armbruster wrote:

{ "sandbox", NULL, QEMU_CAPS_SECCOMP_SANDBOX },


Does option -sandbox exist?

It does since v1.2.  If CONFIG_SECCOMP is off, actually using it is a
fatal error.  Compiling out the option entirely would be more useful, I
guess.

Is this probe still useful?


I believe so.

libvirt adds '-sandbox on' to all VMs it runs, unless the option is not
available.

Some users wanted to run libvirt with QEMUs without libseccomp,
which resulted in the following QEMU commit.

commit 0dd693ef1f15b6e9c4ba8b0118663e10338077cf
sandbox: disable -sandbox if CONFIG_SECCOMP undefined

While using this option won't work if CONFIG_SECCOMP is off,
it should not show up in q-c-l-o so libvirt won't even try to use it.


If I'm reading
 commit 90835c2b8127406615785a9d4348ffdf3c813c8a
 seccomp: convert to meson
correctly, then the whole softmmu/qemu-seccomp.c file is only compiled
if seccomp was found.

Jano



signature.asc
Description: PGP signature

Re: [PATCH v2] i386: Deprecate the -no-hpet QEMU command line option

[Ján Tomko]

On a Thursday in 2022, Thomas Huth wrote:

The HPET setting has been turned into a machine property a while ago
already, so we should finally do the next step and deprecate the
legacy CLI option, too.

Signed-off-by: Thomas Huth 
---
v2:
- Rebased to current version from master branch / adjusted version info
- Dropped the descrpition in hw/i386/pc.c (already done via another patch)

Note: The "hpet" property should now show up in the output of the
"query-command-line-options" QMP command since commit 2f129fc107b4a, so
it should be feasible for libvirt now to properly probe for the property .

docs/about/deprecated.rst | 6 ++
softmmu/vl.c  | 1 +
qemu-options.hx   | 2 +-
3 files changed, 8 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[PATCH for 7.2] Revert "usbredir: avoid queuing hello packet on snapshot restore"

[Ján Tomko]

From: Joelle van Dyne 

Run state is also in RUN_STATE_PRELAUNCH while "-S" is used.

This reverts commit 0631d4b448454ae8a1ab091c447e3f71ab6e088a

Signed-off-by: Joelle van Dyne 
Reviewed-by: Ján Tomko 

The original commit broke the usage of usbredir with libvirt, which
starts every domain with "-S".

This workaround is no longer needed because the usbredir behavior
has been fixed in the meantime:
https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/61

Signed-off-by: Ján Tomko 
---
 hw/usb/redirect.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 1bd30efc3e..fd7df599bc 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1280,8 +1280,7 @@ static void usbredir_create_parser(USBRedirDevice *dev)
 }
 #endif
 
-if (runstate_check(RUN_STATE_INMIGRATE) ||
-runstate_check(RUN_STATE_PRELAUNCH)) {
+if (runstate_check(RUN_STATE_INMIGRATE)) {
 flags |= usbredirparser_fl_no_hello;
 }
 usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE,
-- 
2.38.1

Re: [PATCH 1/3] Revert "usbredir: avoid queuing hello packet on snapshot restore"

[Ján Tomko]

On a Friday in 2022, Joelle van Dyne wrote:

Run state is also in RUN_STATE_PRELAUNCH while "-S" is used.

This reverts commit 12d182898a4866e4be418e2abac286b497cfa1b2.

Signed-off-by: Joelle van Dyne 
---
hw/usb/redirect.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)



Reviewed-by: Ján Tomko 

This fixes usb redirect on VM startup for VMs started by libvirt, which
uses -S:
https://bugzilla.redhat.com/show_bug.cgi?id=2144436

Jano


signature.asc
Description: PGP signature

Re: [PATCH] tests/migration: remove the unused local variable

[Ján Tomko]

On a Wednesday in 2022, dinglimin wrote:

From: "dingli...@cmss.chinamobile.com" 

Remove the unused local variable "records".

Signed-off-by: dinglimin 
---
tests/migration/guestperf/engine.py | 1 -
1 file changed, 1 deletion(-)


Unused since its introduction in 409437e16df273fc5f78f6cd1cb53023eaeb9b72

Reviewed-by: Ján Tomko 

Jano

Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

[Ján Tomko]

On a Wednesday in 2022, Daniel P. Berrangé wrote:

The -device JSON syntax impl leaks a reference on the created
DeviceState instance. As a result when you hot-unplug the
device, the device_finalize method won't be called and thus
it will fail to emit the required DEVICE_DELETED event.

A 'json-cli' feature was previously added against the
'device_add' QMP command QAPI schema to indicated to mgmt
apps that -device supported JSON syntax. Given the hotplug
bug that feature flag is no unusable for its purpose, so
we add a new 'json-cli-hotplug' feature to indicate the
-device supports JSON without breaking hotplug.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/802
Signed-off-by: Daniel P. Berrangé 
---
qapi/qdev.json |  5 -
softmmu/vl.c   |  4 +++-
tests/qtest/device-plug-test.c | 19 +++
3 files changed, 26 insertions(+), 2 deletions(-)



Tested-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [RFC PATCH] blog post: how to get your new feature up-streamed

[Ján Tomko]

On a Friday in 2021, Alex Bennée wrote:

Experience has shown that getting new functionality up-streamed can be
a somewhat painful process. Lets see if we can collect some of our
community knowledge into a blog post describing some best practices
for getting code accepted.

[AJB: obviously RFC for now, need material for the end]

Signed-off-by: Alex Bennée 
---
...26-so-you-want-to-add-something-to-qemu.md | 100 ++
1 file changed, 100 insertions(+)
create mode 100644 _posts/2021-11-26-so-you-want-to-add-something-to-qemu.md

diff --git a/_posts/2021-11-26-so-you-want-to-add-something-to-qemu.md 
b/_posts/2021-11-26-so-you-want-to-add-something-to-qemu.md
new file mode 100644
index 000..d38c0ca
--- /dev/null
+++ b/_posts/2021-11-26-so-you-want-to-add-something-to-qemu.md


[..]


+The maintainers path
+
+


[..]


+I won't pretend there isn't some commitment required when becoming a
+maintainer. However if you were motivated enough to write the code for
+a new feature you should be up to keeping it running smoothly in the
+upstream. The level of effort required is also proportional to the
+popularity of the feature - there is a world of difference between
+maintaining an individual device and a core subsystem. If the feature
+


Unfinished sentence.


+Practically you will probably want to get yourself a
+[GitLab](https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS)
+account so you can run the CI tests on your pull requests. While
+membership of `qemu-devel` is recommended no one is expecting you to
+read every message sent to it as long as you look at those where you
+are explicitly Cc'd.
+
+Now if you are convinced to become a maintainer for your new feature
+lets discuss how you can improve the chances of getting it merged.
+


* let's

Jano


signature.asc
Description: PGP signature

Re: [PATCH] configure: remove dead variables

[Ján Tomko]

On a Friday in 2021, Paolo Bonzini wrote:

Signed-off-by: Paolo Bonzini 
---
configure | 5 -
1 file changed, 5 deletions(-)



Reviewed-by: Ján Tomko 

$supported_os is unused since
commit f9332757898a764d85e19d339ec421236e885b68
meson: move summary to meson.build

$haiku is unused since
commit d99e97e6912d90a55e9a92e004dd54513da2848a
configure: Add a proper check for openpty() in libutil

CONFIG_HAIKU is unused since
commit 4348300e751df1cd24810fb5f699f1f85bbc2849
net/tap: Replace tap-haiku.c and tap-aix.c by a generic tap-stub.c

Jano


signature.asc
Description: PGP signature

Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Ján Tomko]

On a Wednesday in 2021, Daniel P. Berrangé wrote:

We are still adding HMP commands without any QMP counterparts. This is
done because there are a reasonable number of scenarios where the cost
of designing a QAPI data type for the command is not justified.

This has the downside, however, that we will never be able to fully
isolate the monitor code from the remainder of QEMU internals. It is
desirable to be able to get to a point where subsystems in QEMU are
exclusively implemented using QAPI types and never need to have any
knowledge of the monitor APIs.

The way to get there is to stop adding commands to HMP only. All
commands must be implemented using QMP, and any HMP implementation
be a shim around the QMP implementation.

We don't want to compromise our supportability of QMP long term though.

This series proposes that we relax our requirements around fine grained
QAPI data design, but with the caveat that any command taking this
design approach is mandated to use the 'x-' name prefix.

This tradeoff should be suitable for any commands we have been adding
exclusively to HMP in recent times, and thus mean we have mandate QMP
support for all new commands going forward.

This series illustrates the concept by converting the "info registers"
HMP to invoke a new 'x-query-registers' QMP command. Note that only
the i386 CPU target is converted to work with this new approach, so
this series needs to be considered incomplete. If we go forward with
this idea, then a subsequent version of this series would need to
obviously convert all other CPU targets.

After doing that conversion the only use of qemu_fprintf() would be
the disas.c file. Remaining uses of qemu_fprintf and qemu_printf
could be tackled in a similar way and eventually eliminate the need
for any of these printf wrappers in QEMU.

NB: I added docs to devel/writing-qmp-commands.rst about the two
design approaches to QMP. I didn't see another good place to put
an explicit note that we will not add any more HMP-only commands.
Obviously HMP/QMP maintainers control this in their reviews of
patches, and maybe that's sufficient ?

NB: if we take this approach we'll want to figure out how many
HMP-only commands we actually have left and then perhaps have
a task to track their conversion to QMP. This could possibly
be a useful task for newbies if we make it clear that they
wouldn't be required to undertake complex QAPI modelling in
doing this conversion.

Daniel P. Berrangé (5):
 docs/devel: document expectations for QAPI data modelling for QMP
 hw/core: introduce 'format_state' callback to replace 'dump_state'
 target/i386: convert to use format_state instead of dump_state
 qapi: introduce x-query-registers QMP command
 monitor: rewrite 'info registers' in terms of 'x-query-registers'

docs/devel/writing-qmp-commands.rst |  25 +++
hw/core/cpu-common.c|  15 ++
hw/core/machine-qmp-cmds.c  |  28 +++
include/hw/core/cpu.h   |  13 +-
monitor/misc.c  |  25 ++-
qapi/machine.json   |  37 
target/i386/cpu-dump.c  | 325 +++-
target/i386/cpu.c   |   2 +-
target/i386/cpu.h   |   2 +-
9 files changed, 307 insertions(+), 165 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 3/5] target/i386: convert to use format_state instead of dump_state

[Ján Tomko]

On a Wednesday in 2021, Daniel P. Berrangé wrote:

Signed-off-by: Daniel P. Berrangé 
---
target/i386/cpu-dump.c | 325 ++---
target/i386/cpu.c  |   2 +-
target/i386/cpu.h  |   2 +-
3 files changed, 174 insertions(+), 155 deletions(-)

diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
index 02b635a52c..8e19485a20 100644
--- a/target/i386/cpu-dump.c
+++ b/target/i386/cpu-dump.c


[...]


@@ -355,107 +359,116 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int 
flags)


[...]


{
-qemu_fprintf(f, "GDT= %08x %08x\n",
- (uint32_t)env->gdt.base, env->gdt.limit);
-qemu_fprintf(f, "IDT= %08x %08x\n",
- (uint32_t)env->idt.base, env->idt.limit);
-qemu_fprintf(f, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n",
- (uint32_t)env->cr[0],
- (uint32_t)env->cr[2],
- (uint32_t)env->cr[3],
- (uint32_t)env->cr[4]);
+g_string_append_printf(buf, "GDT= %08x %08x\n",
+   (uint32_t)env->gdt.base, env->gdt.limit);
+g_string_append_printf(buf, "IDT= %08x %08x\n",
+   (uint32_t)env->idt.base, env->idt.limit);
+g_string_append_printf(buf, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n",
+   (uint32_t)env->cr[0],
+   (uint32_t)env->cr[2],
+   (uint32_t)env->cr[3],
+   (uint32_t)env->cr[4]);
for(i = 0; i < 4; i++) {
-qemu_fprintf(f, "DR%d=" TARGET_FMT_lx " ", i, env->dr[i]);
+g_string_append_printf(buf, "DR%d=" TARGET_FMT_lx
+   " ", i, env->dr[i]);


The formatting string can comfortably fit on the first line here.

Jano


}
-qemu_fprintf(f, "\nDR6=" TARGET_FMT_lx " DR7=" TARGET_FMT_lx "\n",
- env->dr[6], env->dr[7]);
+g_string_append_printf(buf, "\nDR6=" TARGET_FMT_lx
+   " DR7=" TARGET_FMT_lx "\n",
+   env->dr[6], env->dr[7]);


signature.asc
Description: PGP signature

Re: [PATCH] hw/arm/raspi: Remove deprecated raspi2/raspi3 aliases

[Ján Tomko]

On a Monday in 2021, Philippe Mathieu-Daudé wrote:

Remove the raspi2/raspi3 machine aliases,
deprecated since commit 155e1c82ed0.

Signed-off-by: Philippe Mathieu-Daudé 
---
docs/system/deprecated.rst   | 7 ---
docs/system/removed-features.rst | 7 +++
hw/arm/raspi.c   | 2 --
3 files changed, 7 insertions(+), 9 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] accel/tcg: Remove deprecated '-tb-size' option

[Ján Tomko]

On a Wednesday in 2020, Philippe Mathieu-Daudé wrote:

The '-tb-size' option (replaced by '-accel tcg,tb-size') is
deprecated since 5.0 (commit fe174132478). Remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
docs/system/deprecated.rst | 12 +---
accel/tcg/translate-all.c  |  2 +-
softmmu/vl.c   |  8 
qemu-options.hx|  8 
4 files changed, 6 insertions(+), 24 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] target/ppc: Remove "compat" property of server class POWER CPUs

[Ján Tomko]

On a Tuesday in 2020, Greg Kurz wrote:

This property has been deprecated since QEMU 5.0 by commit 22062e54bb68.
We only kept a legacy hack that internally converts "compat" into the
official "max-cpu-compat" property of the pseries machine type.

According to our deprecation policy, we could have removed it for QEMU 5.2
already. Do it now ; since ppc_cpu_parse_featurestr() now just calls the
generic parent_parse_features handler, drop it as well.

Users are supposed to use the "max-cpu-compat" property of the pseries
machine type instead.



For libvirt:
Reviewed-by: Ján Tomko 

We use the new option as of libvirt commit:

commit 2b041dc8c7b70e762d99b6bd7805daa9961740f6
Author: Shivaprasad G Bhat 
AuthorDate: 2018-01-05 19:18:00 +0530
Commit: Andrea Bolognani 
CommitDate: 2018-01-05 17:12:14 +0100

qemu: Add support for pseries machine's max-cpu-compat= parameter

Jano


Signed-off-by: Greg Kurz 
---
docs/system/deprecated.rst  |  7 
target/ppc/translate_init.c.inc | 59 -
2 files changed, 66 deletions(-)



signature.asc
Description: PGP signature

Re: [PATCH v2 6/6] tools/virtiofsd: xattr name mapping examples

[Ján Tomko]

On a Thursday in 2020, Dr. David Alan Gilbert (git) wrote:

From: "Dr. David Alan Gilbert" 

Add a few examples of xattrmaps to the documentation.

Signed-off-by: Dr. David Alan Gilbert 
---
docs/tools/virtiofsd.rst | 49 
1 file changed, 49 insertions(+)

diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst
index 2efa16d3c5..a138549862 100644
--- a/docs/tools/virtiofsd.rst
+++ b/docs/tools/virtiofsd.rst
@@ -161,6 +161,55 @@ in which case a 'server' rule will always match on all 
names from
the server.


+xattr-mapping Examples
+--
+
+1) Prefix all attributes with 'user.virtiofs.'
+
+::
+
+-o xattrmap=":all:prefix::user.virtiofs.::all:bad:::"
+
+
+This uses two rules, using : as the field separator;
+the first rule prefixes and strips 'user.virtiofs.',
+the second rule hides any non-prefixed attributes that
+the host set.
+
+2) Prefix 'trusted.' attributes, allow others through
+
+::
+
+   "/all/prefix/trusted./user.virtiofs./
+/server/bad//trusted./
+/client/bad/user.virtiofs.trusted.//
+/all/ok///"
+
+
+Here there are four rules, using / as the field
+separator, and also demonstrating that new lines can
+be included between rules.
+The first rule is the prefixing of 'trusted.'.
+The second rule hides unprefixed 'trusted.' attributes
+on the host.
+The third rule stops a guest from explicitily setting


explicitly


+the 'user.viritofs.trusted.' path directly.
+Finally, the fourth rule lets all remaining attributes
+through.
+
+3) Hide 'security.' attributes, and allow everything else
+
+::
+
+"/all/bad/security./security./
+ /all/ok///'
+
+The first rule combines what could be separate client and server
+rules into a single 'all' rule, matching 'security.' in either
+client arguments or lists returned from the host.  This stops
+the client seeing any 'security.' attributes on the server and
+stops it setting  any.


extra space.

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 3/6] tools/virtiofsd: xattr name mappings: Add option

[Ján Tomko]

On a Thursday in 2020, Dr. David Alan Gilbert (git) wrote:

From: "Dr. David Alan Gilbert" 

Add an option to define mappings of xattr names so that
the client and server filesystems see different views.
This can be used to have different SELinux mappings as
seen by the guest, to run the virtiofsd with less privileges
(e.g. in a case where it can't set trusted/system/security
xattrs but you want the guest to be able to), or to isolate
multiple users of the same name; e.g. trusted attributes
used by stacking overlayfs.

A mapping engine is used wit 3 simple rules; the rules can
be combined to allow most useful mapping scenarios.
The ruleset is defined by -o xattrmap='rules...'.

This patch doesn't use the rule maps yet.

Signed-off-by: Dr. David Alan Gilbert 
---
docs/tools/virtiofsd.rst |  55 
tools/virtiofsd/passthrough_ll.c | 148 +++
2 files changed, 203 insertions(+)

diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst
index 824e713491..2efa16d3c5 100644
--- a/docs/tools/virtiofsd.rst
+++ b/docs/tools/virtiofsd.rst
@@ -107,6 +107,60 @@ Options
  performance.  ``auto`` acts similar to NFS with a 1 second metadata cache
  timeout.  ``always`` sets a long cache lifetime at the expense of coherency.

+xattr-mapping
+-
+
+By default the name of xattr's used by the client are passed through to the 
server
+file system.  This can be a problem where either those xattr names are used
+by something on the server (e.g. selinux client/server confusion) or if the
+virtiofsd is running in a container with restricted priviliges where it cannot


privileges


+access some attributes.
+
+A mapping of xattr names can be made using -o xattrmap=mapping where the 
``mapping``
+string consists of a series of rules.
+
+The first matching rule terminates the mapping.
+
+Each rule consists of a number of fields separated with a separator that is the
+first non-white space character in the rule.  This separator must then be used
+for the whole rule.
+White space may be added before and after each rule.
+Using ':' as the separator a rule is of the form:
+
+``:scope:type:key:prepend:``
+
+**scope** is:
+
+- 'client' - match 'key' against a xattr name from the client for
+ setxattr/getxattr/removexattr
+- 'server' - match 'prepend' against a xattr name from the server
+ for listxattr
+- 'all' - can be used to match both cases.
+
+**type** is one of:
+
+- 'prefix' - If 'key' matches the client then the 'prepend'
+  is added before the name is passed to the server.
+  For a server case, the prepend is tested and stripped
+  if matching.
+
+- 'ok' - The attribute name is OK and passed through to
+  the server unchanged.
+
+- 'bad' - If a client tries to use this name it's
+  denied using EPERM; when the server passes an attribute
+  name matching it's hidden.
+
+**key** is a string tested as a prefix on an attribute name originating
+on the client.  It maybe empty in which case a 'client' rule
+will always match on client names.
+
+**prepend** is a string tested as a prefix on an attribute name originiating


originating


+on the server, and used as a new prefix.  It maybe empty


may be


+in which case a 'server' rule will always match on all names from
+the server.
+
+
Examples


@@ -123,3 +177,4 @@ Export ``/var/lib/fs/vm001/`` on vhost-user UNIX domain 
socket
  -numa node,memdev=mem \
  ...
  guest# mount -t virtiofs myfs /mnt
+


git complains about trailing whitespace at EOF

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 1/6] virtiofsd: Silence gcc warning

[Ján Tomko]

On a Thursday in 2020, Dr. David Alan Gilbert (git) wrote:

From: "Dr. David Alan Gilbert" 

Gcc worries fd might be used unset, in reality it's always set if
fi is set, and only used if fi is set so it's safe.  Initialise it to -1
just to keep gcc happy for now.

Signed-off-by: Dr. David Alan Gilbert 
---
tools/virtiofsd/passthrough_ll.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 11/13] audio: deprecate -soundhw pcspk

[Ján Tomko]

On a Monday in 2020, Daniel P. Berrangé wrote:

On Mon, May 18, 2020 at 12:16:28PM +0200, Gerd Hoffmann wrote:

On Fri, May 15, 2020 at 05:08:23PM +0200, Ján Tomko wrote:
> On a Friday in 2020, Gerd Hoffmann wrote:
> > Add deprecation message to the audio init function.
> >
> > Factor out audio initialization and call that from
> > both audio init and realize, so setting audiodev via
> > -global is enough to properly initialize pcspk.
> >
> > Signed-off-by: Gerd Hoffmann 
> > ---
> > hw/audio/pcspk.c | 24 +---
> > 1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > @@ -236,9 +245,18 @@ static const TypeInfo pcspk_info = {
> > .class_init = pcspk_class_initfn,
> > };
> >
> > +static int pcspk_audio_init_soundhw(ISABus *bus)
> > +{
> > +PCSpkState *s = pcspk_state;
> > +
> > +warn_report("'-soundhw pcspk' is deprecated, "
> > +"please set a backend using '-global isa-pcspk.audiodev=' 
instead");
> > +return pcspk_audio_init(s);
>
> -soundhw pcspk is the only soundhw device present in libvirt git.
>
> Is there a way to probe for this change via QMP?

Oops.  I'm surprised libvirt actually supports pcspk.

There is no way to see that in qmp, and I can't think of an easy way
to add that.  Does libvirt check for command line switches still?
So it could see -soundhw going away if that happens?


IIUC, instead of probing for whether -soundhw is deprecated, it should
be suffiicent for us to probe if "isa-pcspk.audiodev" exists. Assuming
we always use isa-pcspk.audiodev if it exists, then we'll trivially
avoid using the -soundhw arg.



Yes, we can probe for that, but the phrasing in the commit message makes
it look like setting the property via -global will only be effective
after this commit.

Jano


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



signature.asc
Description: PGP signature

Re: [PATCH v2 11/13] audio: deprecate -soundhw pcspk

[Ján Tomko]

On a Friday in 2020, Gerd Hoffmann wrote:

Add deprecation message to the audio init function.

Factor out audio initialization and call that from
both audio init and realize, so setting audiodev via
-global is enough to properly initialize pcspk.

Signed-off-by: Gerd Hoffmann 
---
hw/audio/pcspk.c | 24 +---
1 file changed, 21 insertions(+), 3 deletions(-)

@@ -236,9 +245,18 @@ static const TypeInfo pcspk_info = {
.class_init = pcspk_class_initfn,
};

+static int pcspk_audio_init_soundhw(ISABus *bus)
+{
+PCSpkState *s = pcspk_state;
+
+warn_report("'-soundhw pcspk' is deprecated, "
+"please set a backend using '-global isa-pcspk.audiodev=' 
instead");
+return pcspk_audio_init(s);


-soundhw pcspk is the only soundhw device present in libvirt git.

Is there a way to probe for this change via QMP?

Jano


+}
+
static void pcspk_register(void)
{
type_register_static(_info);
-isa_register_soundhw("pcspk", "PC speaker", pcspk_audio_init);
+isa_register_soundhw("pcspk", "PC speaker", pcspk_audio_init_soundhw);
}
type_init(pcspk_register)
--
2.18.4



signature.asc
Description: PGP signature

Re: [PATCH 1/1] conf: qemu 9pfs: add 'multidevs' option

[Ján Tomko]

On a Thursday in 2020, Christian Schoenebeck wrote:

On Donnerstag, 19. März 2020 14:10:26 CET Ján Tomko wrote:

On a Tuesday in 2020, Christian Schoenebeck wrote:
>Introduce new 'multidevs' option for filesystem.
>
>  

I don't like the 'multidevs' name, but cannot think of anything
beter.

'collisions' maybe?


Not sure if 'collisions' is better, e.g. collisions='remap' sounds scary. :)
And which collision would that be? At least IMO 'multidevs' is less ambigious.
I have no problem though to change it to whatever name you might come up with.
Just keep the resulting key-value pair set in mind:

multidevs='default'
multidevs='remap'
multidevs='forbid'
multidevs='warn'

vs.

collisions='default'
collisions='remap' <- probably misleading what 'remap' means in this case
collisions='forbid'
collisions='warn' <- wrong, it warns about multiple devices, not about file ID
collisions.

So different key-name might also require different value-names.

Another option would be the long form 'multi-devices=...'


Okay, let's leave it at 'multidevs'.




>
>
>
>  
>
>This option prevents misbheaviours on guest if a 9pfs export
>contains multiple devices, due to the potential file ID collisions
>this otherwise may cause.
>
>Signed-off-by: Christian Schoenebeck 
>---
>
> docs/formatdomain.html.in | 47 ++-
> docs/schemas/domaincommon.rng | 10 
> src/conf/domain_conf.c| 30 ++
> src/conf/domain_conf.h| 13 ++
> src/qemu/qemu_command.c   |  7 ++
> 5 files changed, 106 insertions(+), 1 deletion(-)

Please split the XML changes from the qemu driver changes.


Ok


Also missing:
* qemu_capabilities addition


AFAICS the common procedure is to add new capabilities always to the end of
the enum list. So I guess I will do that as well.


* qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability,
reject this setting for virtiofs


Good to know where that check is supposed to go to, thanks!


* qemuxml2xmltest addition
* qemuxml2argvtest addition


Ok, I have to read up how those tests work. AFAICS I need to add xml files to
their data subdirs.

Separate patches required for those 2 tests?


Usually xml2xmltest is in the same test as the XML parser/formatter
and xml2argvtest is a part of the qemu driver patch.

Jano


signature.asc
Description: PGP signature

Re: [PATCH 1/1] conf: qemu 9pfs: add 'multidevs' option

[Ján Tomko]

On a Tuesday in 2020, Christian Schoenebeck wrote:

Introduce new 'multidevs' option for filesystem.

 


I don't like the 'multidevs' name, but cannot think of anything
beter.

'collisions' maybe?


   
   
 

This option prevents misbheaviours on guest if a 9pfs export
contains multiple devices, due to the potential file ID collisions
this otherwise may cause.

Signed-off-by: Christian Schoenebeck 
---
docs/formatdomain.html.in | 47 ++-
docs/schemas/domaincommon.rng | 10 
src/conf/domain_conf.c| 30 ++
src/conf/domain_conf.h| 13 ++
src/qemu/qemu_command.c   |  7 ++
5 files changed, 106 insertions(+), 1 deletion(-)


Please split the XML changes from the qemu driver changes.

Also missing:
* qemu_capabilities addition
* qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability,
  reject this setting for virtiofs
* qemuxml2xmltest addition
* qemuxml2argvtest addition

(no changes required for virschematest - it checks all the XML files in
the directories used by the above tests against the schema)



diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 594146009d..13c506988b 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3967,7 +3967,7 @@
source name='my-vm-template'/
target dir='/'/
  /filesystem
-  filesystem type='mount' accessmode='passthrough'
+  filesystem type='mount' accessmode='passthrough' multidevs='remap'
driver type='path' wrpolicy='immediate'/
source dir='/export/to/guest'/
target dir='/import/from/host'/
@@ -4084,13 +4084,58 @@



+  
  Since 5.2.0, the filesystem element
  has an optional attribute model with supported values
  "virtio-transitional", "virtio-non-transitional", or "virtio".
  See Virtio transitional devices
  for more details.
+  
+


Unrelated change that can be split out.


+  
+  The filesystem element has an optional attribute multidevs
+  which specifies how to deal with a filesystem export containing more than
+  one device, in order to avoid file ID collisions on guest when using 9pfs
+  (since 6.2.0, requires QEMU 4.2).
+  This attribute is not available for virtiofs. The possible values are:
+  
+
+
+default
+
+Use QEMU's default setting (which currently is warn).
+
+remap
+
+This setting allows guest to access multiple devices per export without
+encountering misbehaviours. Inode numbers from host are automatically
+remapped on guest to actively prevent file ID collisions if guest
+accesses one export containing multiple devices.
+
+forbid
+
+Only allow to access one device per export by guest. Attempts to access
+additional devices on the same export will cause the individual
+filesystem access by guest to fail with an error and being logged 
(once)
+as error on host side.
+
+warn
+
+This setting resembles the behaviour of 9pfs prior to QEMU 4.2, that is
+no action is performed to prevent any potential file ID collisions if 
an
+export contains multiple devices, with the only exception: a warning is
+logged (once) on host side now. This setting may lead to misbehaviours
+on guest side if more than one device is exported per export, due to 
the
+potential file ID collisions this may cause on guest side in that case.
+
+
+
  




+  
+  The filesystem element may contain the following 
subelements:
+  
+


And so can this one.


  driver
  
The optional driver element allows specifying further details
@@ -25422,6 +25449,9 @@ virDomainFSDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, " model='%s'",
  virDomainFSModelTypeToString(def->model));
}
+if (def->multidevs) {
+virBufferAsprintf(buf, " multidevs='%s'", multidevs);
+}


make syntax-check complains here:
Curly brackets around single-line body:
../src/conf/domain_conf.c:25452-25454:
if (def->multidevs) {
virBufferAsprintf(buf, " multidevs='%s'", multidevs);
}

Jano


signature.asc
Description: PGP signature

Re: [PATCH] util: fix to get configuration macros in util/mmap-alloc.c

[Ján Tomko]

On a Thursday in 2020, Jingqi Liu wrote:

The CONFIG_LINUX symbol is always not defined in this file.
This fixes that "config-host.h" header file is not included
for getting macros.

Signed-off-by: Jingqi Liu 
---
util/mmap-alloc.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
index 27dcccd8ec..24c0e380f3 100644
--- a/util/mmap-alloc.c
+++ b/util/mmap-alloc.c
@@ -10,6 +10,8 @@
 * later.  See the COPYING file in the top-level directory.
 */

+#include "config-host.h"
+


According to CODING_STYLE.rst, qemu/osdep.h is the header file
that should be included first, before all the other includes.

So the minimal fix would be moving qemu/osdep.h up here.


#ifdef CONFIG_LINUX
#include 
#else  /* !CONFIG_LINUX */



Introduced by commit 119906afa5ca610adb87c55ab0d8e53c9104bfc3

Jano


--
2.17.1




signature.asc
Description: PGP signature

Re: [PATCH] block/qcow2-threads: fix qcow2_decompress

[Ján Tomko]

On a Monday in 2020, Vladimir Sementsov-Ogievskiy wrote:

On success path we return what inflate() returns instead of 0. And it
most probably works for Z_STREAM_END as it is positive, but is
definitely broken for Z_BUF_ERROR.

While being here, switch to errno return code, to be closer to
qcow2_compress API (and usual expectations).

Revert condition in if to be more positive. Drop dead initialization of
ret.

Cc: qemu-sta...@nongnu.org # v4.0
Fixes: 341926ab83e2b
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi!

Reviewing Den's series about zstd in qcow2 support, I found an existing
bug. Let's fix it. This is to be a new base of zstd series.

block/qcow2-threads.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] vhost-vsock: fix error message output

[Ján Tomko]

On a Sunday in 2020, Nick Erdmann wrote:

error_setg_errno takes a positive error number, so we should not invert
errno's sign.

Signed-off-by: Nick Erdmann 
---
hw/virtio/vhost-vsock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [RFC PATCH] linux-user/syscall: Use g_file_open_tmp()

[Ján Tomko]

On a Thursday in 2020, Daniel P. Berrangé wrote:

On Thu, Feb 27, 2020 at 11:06:21AM +0100, Philippe Mathieu-Daudé wrote:

Use GLib g_file_open_tmp() instead of getenv + snprintf + mkstemp.

Signed-off-by: Philippe Mathieu-Daudé 
---
RFC because I'm not sure g_autoptr(GError) works this way.


It does work. Any struct that's defined in GLib has support for
g_autoptr(). If you aren't suyre though, just check for a
G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro usage that refers to the
struct in question

$ grep -r 'G_DEFINE_AUTOPTR_CLEANUP_FUNC(GError' /usr/include/glib-2.0
/usr/include/glib-2.0/glib/glib-autocleanups.h:G_DEFINE_AUTOPTR_CLEANUP_FUNC(GError,
 g_error_free)


 linux-user/syscall.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 8d27d10807..0e44969e16 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7282,17 +7282,14 @@ static int do_openat(void *cpu_env, int dirfd, const 
char *pathname, int flags,
 }

 if (fake_open->filename) {
-const char *tmpdir;
-char filename[PATH_MAX];
+g_autoptr(GError) gerr = NULL;
+g_autofree gchar *filename = NULL;
 int fd, r;

 /* create temporary file to map stat to */
-tmpdir = getenv("TMPDIR");
-if (!tmpdir)
-tmpdir = "/tmp";
-snprintf(filename, sizeof(filename), "%s/qemu-open.XX", tmpdir);
-fd = mkstemp(filename);
+fd = g_file_open_tmp("qemu-open.XX", , );


g_file_open_tmp, calls g_get_tmp_name, which calls
g_get_tmp_dir, which defaults to $TMPDIR, falling back
to /tmp. So we're using the same dir as before.


 if (fd < 0) {
+fprintf(stderr, "Error opening %s: %s\n", filename, gerr->message);


This is wrong - the returned "filename" is only valid when
g_file_open_tmp succeeds. So the use of "filename" here
is likely a NULL. Given that the only place you use "filename"
is in the error path, and that's not valid, we can simply
eliminate it entirely, and pass NULL into g_file_open_tmp



'filename' is used right below to unlink it.
But it can be dropped from the error message here since it's included in the
error reported by GLib.

Jano


 return fd;
 }
 unlink(filename);


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




signature.asc
Description: PGP signature

Re: [PATCH v2 3/3] qemu-img: Deprecate use of -b without -F

[Ján Tomko]

On a Wednesday in 2020, Eric Blake wrote:

Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on
probes leaking information to the guest on a subsequent boot).  If our
probing algorithm ever changes, or if other tools like libvirt
determine a different probe result than we do, then subsequent use of
that backing file under a different format will present corrupted data
to the guest.  Start a deprecation clock so that future qemu-img can
refuse to create unsafe backing chains that would rely on probing.

However, there is one time where probing is safe: if we probe raw,
then it is safe to record that implicitly in the image (but we still
warn, as it's better to teach the user to supply -F always than to
make them guess when it is safe).

iotest 114 specifically wants to create an unsafe image for later
amendment rather than defaulting to our new default of recording a
probed format, so it needs an update.

Signed-off-by: Eric Blake 
---
qemu-deprecated.texi   | 15 +++
block.c| 21 -
qemu-img.c |  8 +++-
tests/qemu-iotests/114 |  4 ++--
tests/qemu-iotests/114.out |  1 +
5 files changed, 45 insertions(+), 4 deletions(-)



This seems to affect code paths that are used even outside of qemu-img,
should the commit message mention it?

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 2/3] block: Add support to warn on backing file change without format

[Ján Tomko]

On a Wednesday in 2020, Eric Blake wrote:

For now, this is a mechanical addition; all callers pass false. But
the next patch will use it to improve 'qemu-img rebase -u' when
selecting a backing file with no format.

Signed-off-by: Eric Blake 
---
include/block/block.h |  4 ++--
block.c   | 13 ++---
block/qcow2.c |  2 +-
block/stream.c|  2 +-
blockdev.c|  3 ++-
qemu-img.c|  4 ++--
6 files changed, 18 insertions(+), 10 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] iotests: Specify explicit backing format where sensible

[Ján Tomko]

On a Wednesday in 2020, Eric Blake wrote:

There are many existing qcow2 images that specify a backing file but
no format.  This has been the source of CVEs in the past, but has
become more prominent of a problem now that libvirt has switched to
-blockdev.  With older -drive, at least the probing was always done by
qemu (so the only risk of a changed format between successive boots of
a guest was if qemu was upgraded and probed differently).  But with
newer -blockdev, libvirt must specify a format; if libvirt guesses raw
where the image was formatted, this results in data corruption visible
to the guest; conversely, if libvirt guesses qcow2 where qemu was
using raw, this can result in potential security holes, so modern
libvirt instead refuses to use images without explicit backing format.

The change in libvirt to reject images without explicit backing format
has pointed out that a number of tools have been far too reliant on
probing in the past.  It's time to set a better example in our own
iotests of properly setting this parameter.

iotest calls to create, rebase, convert, and amend are all impacted to
some degree.  It's a bit annoying that we are inconsistent on command
line - while all of those accept -o backing_file=...,backing_fmt=...,
the shortcuts are different: create and rebase have -b and -F, convert
has -B but no -F, and amend has no shortcuts.

Signed-off-by: Eric Blake 
---


[...]

Test #225 still uses -b without a format:

./check -vmdk 225
QEMU  -- 
"/home/jtomko/work/qemu/build/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
 -nodefaults -display none -accel qtest
QEMU_IMG  -- "/home/jtomko/work/qemu/build/tests/qemu-iotests/../../qemu-img" 
QEMU_IO   -- "/home/jtomko/work/qemu/build/tests/qemu-iotests/../../qemu-io"  --cache writeback --aio threads -f vmdk
QEMU_NBD  -- "/home/jtomko/work/qemu/build/tests/qemu-iotests/../../qemu-nbd" 
IMGFMT-- vmdk

IMGPROTO  -- file
PLATFORM  -- Linux/x86_64 lpt 5.4.18-200.fc31.x86_64
TEST_DIR  -- /home/jtomko/work/qemu/build/tests/qemu-iotests/scratch
SOCK_DIR  -- /tmp/tmp.OQIdhLcITP
SOCKET_SCM_HELPER -- 
/home/jtomko/work/qemu/build/tests/qemu-iotests/socket_scm_helper

225  fail   [10:02:31] [10:02:32]output mismatch 
(see 225.out.bad)
--- /home/jtomko/work/qemu/tests/qemu-iotests/225.out   2018-09-07 
17:21:39.633931691 +0200
+++ /home/jtomko/work/qemu/build/tests/qemu-iotests/225.out.bad 2020-02-27 
10:02:32.362755677 +0100
@@ -1,6 +1,7 @@
 QA output created by 225
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
 Formatting 'TEST_DIR/t.IMGFMT.not_base', fmt=IMGFMT size=1048576
+qemu-img: warning: Deprecated use of backing file without explicit backing 
format (detected format of IMGFMT)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.IMGFMT.base
 
 === Testing fitting VMDK backing image ===

Failures: 225
Failed 1 of 1 iotests


diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index aa911d266a13..322e31e2cd93 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -32,8 +32,12 @@ class TestSingleDrive(iotests.QMPTestCase):

def setUp(self):
iotests.create_image(backing_img, TestSingleDrive.image_len)
-qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
backing_img, mid_img)
-qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
mid_img, test_img)
+qemu_img('create', '-f', iotests.imgfmt,
+ '-o', 'backing_file=%s' % backing_img,
+ '-F', 'raw', mid_img)
+qemu_img('create', '-f', iotests.imgfmt,
+ '-o', 'backing_file=%s' % mid_img,
+ '-F', iotests.imgfmt, test_img)


Consider not mixing shortcuts with -o options.


qemu_io('-f', 'raw', '-c', 'write -P 0x1 0 512', backing_img)
qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 524288 512', mid_img)
self.vm = iotests.VM().add_drive("blkdebug::" + test_img,


With test #225 fixed:
Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] iotests: Specify explicit backing format where sensible

[Ján Tomko]

On a Thursday in 2020, Peter Krempa wrote:

On Wed, Feb 26, 2020 at 20:39:26 -0600, Eric Blake wrote:

There are many existing qcow2 images that specify a backing file but
no format.  This has been the source of CVEs in the past, but has
become more prominent of a problem now that libvirt has switched to
-blockdev.  With older -drive, at least the probing was always done by
qemu (so the only risk of a changed format between successive boots of
a guest was if qemu was upgraded and probed differently).  But with
newer -blockdev, libvirt must specify a format; if libvirt guesses raw
where the image was formatted, this results in data corruption visible
to the guest; conversely, if libvirt guesses qcow2 where qemu was
using raw, this can result in potential security holes, so modern
libvirt instead refuses to use images without explicit backing format.

The change in libvirt to reject images without explicit backing format
has pointed out that a number of tools have been far too reliant on
probing in the past.  It's time to set a better example in our own
iotests of properly setting this parameter.

iotest calls to create, rebase, convert, and amend are all impacted to
some degree.  It's a bit annoying that we are inconsistent on command
line - while all of those accept -o backing_file=...,backing_fmt=...,
the shortcuts are different: create and rebase have -b and -F, convert
has -B but no -F, and amend has no shortcuts.

Signed-off-by: Eric Blake 
---


[...]


 113 files changed, 414 insertions(+), 338 deletions(-)

diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
index 0a4b854e6520..585512bb296b 100755
--- a/tests/qemu-iotests/017
+++ b/tests/qemu-iotests/017
@@ -66,7 +66,7 @@ echo "Creating test image with backing file"
 echo

 TEST_IMG=$TEST_IMG_SAVE
-_make_test_img -b "$TEST_IMG.base" 6G
+_make_test_img -b "$TEST_IMG.base" -F $IMGFMT 6G



My understanding of the intricacies of the qemu-iotest suite is not good
enoug to be able to review this patch. Specifically $IMGFMT in this
instance is also used in the '-f' switch of qemu-img in _make_test_img
and I don't know if it's expected for the backing file to share the
format.


IMGFMT is also used for the earlier creation of the base image and
I did not see it changing in any of the tests.

Jano


signature.asc
Description: PGP signature

Re: [PATCH v3] pcie_root_port: Add hotplug disabling option

[Ján Tomko]

On a Wednesday in 2020, Julia Suvorova wrote:

Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt
manage it and restrict unplug for the whole machine. This is going to
prevent user-initiated unplug in guests (Windows mostly).
Hotplug is enabled by default.
Usage:
   -device pcie-root-port,hotplug=off,...

If you want to disable hot-unplug on some downstream ports of one
switch, disable hot-unplug on PCIe Root Port connected to the upstream
port as well as on the selected downstream ports.

Discussion related:
   https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00530.html

Signed-off-by: Julia Suvorova 
---
v1: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04868.html

v2: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg05192.html
   * change name of the option to 'enable-hotplug' [Laine]
   * change order of enabling capability bits [Igor]
   * enable HPS bit [Igor]
   * add option to xio3130_downstream [Ján]

v3:
   * change name of the option to 'hotplug'. Naming is hard! [Laine]
   * move property under TYPE_PCIE_SLOT [Michael]

hw/pci-bridge/pcie_root_port.c |  2 +-
hw/pci-bridge/xio3130_downstream.c |  2 +-
hw/pci/pcie.c  | 11 +++
hw/pci/pcie_port.c |  1 +
include/hw/pci/pcie.h  |  2 +-
include/hw/pci/pcie_port.h |  3 +++
6 files changed, 14 insertions(+), 7 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2] pcie_root_port: Add enable_hotplug option

[Ján Tomko]

On Wed, Feb 19, 2020 at 03:55:40PM +0100, Julia Suvorova wrote:

Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt
manage it and restrict unplug for the whole machine. This is going to
prevent user-initiated unplug in guests (Windows mostly).
Hotplug is enabled by default.
Usage:
   -device pcie-root-port,enable-hotplug=false,...

If you want to disable hot-unplug on some downstream ports of one
switch, disable hot-unplug on PCIe Root Port connected to the upstream
port as well as on the selected downstream ports.

Discussion related:
   https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00530.html

Signed-off-by: Julia Suvorova 
---
v1: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04868.html

v2:
   * change name of the option to 'enable-hotplug' [Laine]
   * change order of enabling capability bits [Igor]
   * enable HPS bit [Igor]
   * add option to xio3130_downstream [Ján]

hw/pci-bridge/pcie_root_port.c |  3 ++-
hw/pci-bridge/xio3130_downstream.c |  3 ++-
hw/pci/pcie.c  | 11 +++
include/hw/pci/pcie.h  |  2 +-
include/hw/pci/pcie_port.h |  1 +
5 files changed, 13 insertions(+), 7 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] pcie_root_port: Add disable_hotplug option

[Ján Tomko]

On Tue, Feb 18, 2020 at 05:17:17PM +0100, Julia Suvorova wrote:

Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt
to manage it and restrict unplug for the entire machine. This is going
to prevent user-initiated unplug in guests (Windows mostly).
Usage:
   -device pcie-root-port,disable-hotplug=true,...

Discussion related:
   https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00530.html

Signed-off-by: Julia Suvorova 
---
hw/core/machine.c  | 1 +
hw/pci-bridge/pcie_root_port.c | 3 ++-
hw/pci-bridge/xio3130_downstream.c | 2 +-
hw/pci/pcie.c  | 8 ++--
include/hw/pci/pcie.h  | 2 +-
include/hw/pci/pcie_port.h | 1 +
6 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 84812a1d1c..5ff698ac3c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -36,6 +36,7 @@ GlobalProperty hw_compat_4_2[] = {
{ "usb-redir", "suppress-remote-wake", "off" },
{ "qxl", "revision", "4" },
{ "qxl-vga", "revision", "4" },
+{ "pcie-root-port-base", "disable-hotplug", "false" },
};
const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);

diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 0ba4e4dea4..d6a080bee8 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -94,7 +94,7 @@ static void rp_realize(PCIDevice *d, Error **errp)

pcie_cap_arifwd_init(d);
pcie_cap_deverr_init(d);
-pcie_cap_slot_init(d, s->slot);
+pcie_cap_slot_init(d, s);
pcie_cap_root_init(d);

pcie_chassis_create(s->chassis);
@@ -147,6 +147,7 @@ static Property rp_props[] = {
DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
QEMU_PCIE_SLTCAP_PCP_BITNR, true),
DEFINE_PROP_BOOL("disable-acs", PCIESlot, disable_acs, false),
+DEFINE_PROP_BOOL("disable-hotplug", PCIESlot, disable_hotplug, false),
DEFINE_PROP_END_OF_LIST()
};

diff --git a/hw/pci-bridge/xio3130_downstream.c 
b/hw/pci-bridge/xio3130_downstream.c
index 153a4acad2..04aae72cd6 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -94,7 +94,7 @@ static void xio3130_downstream_realize(PCIDevice *d, Error 
**errp)
}
pcie_cap_flr_init(d);
pcie_cap_deverr_init(d);
-pcie_cap_slot_init(d, s->slot);
+pcie_cap_slot_init(d, s);
pcie_cap_arifwd_init(d);



The corresponding entry in xio3130_downstream_props[] is missing.


pcie_chassis_create(s->chassis);


Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 2/3] tools/virtiofsd/passthrough_ll: Remove unneeded variable assignment

[Ján Tomko]

On Mon, Feb 17, 2020 at 10:42:39AM +0100, Philippe Mathieu-Daudé wrote:

Fix warning reported by Clang static code analyzer:

   CC  tools/virtiofsd/passthrough_ll.o
 tools/virtiofsd/passthrough_ll.c:925:9: warning: Value stored to 'newfd' is 
never read
 newfd = -1;
 ^   ~~
 tools/virtiofsd/passthrough_ll.c:942:9: warning: Value stored to 'newfd' is 
never read
 newfd = -1;
 ^   ~~

Fixes: 7c6b66027
Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: do not set newfd, use it (jtomko)
---
tools/virtiofsd/passthrough_ll.c | 2 --
1 file changed, 2 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 4/7] commit: Inline commit_populate()

[Ján Tomko]

On Fri, Feb 14, 2020 at 09:08:09PM +0100, Kevin Wolf wrote:

commit_populate() is a very short function and only called in a single
place. Its return value doesn't tell us whether an error happened while
reading or writing, which would be necessary for sending the right data
in the BLOCK_JOB_ERROR QMP event.

Signed-off-by: Kevin Wolf 
---
block/commit.c | 28 ++--
1 file changed, 6 insertions(+), 22 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 6/7] commit: Expose on-error option in QMP

[Ján Tomko]

On Fri, Feb 14, 2020 at 09:08:11PM +0100, Kevin Wolf wrote:

Now that the error handling in the common block job is fixed, we can
expose the on-error option in QMP instead of hard-coding it as 'report'
in qmp_block_commit().

This fulfills the promise that the old comment in that function made,
even if a bit later than expected: "This will be part of the QMP
command, if/when the BlockdevOnError change for blkmirror makes it in".

Signed-off-by: Kevin Wolf 
---
qapi/block-core.json | 4 
blockdev.c   | 8 
2 files changed, 8 insertions(+), 4 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 5/7] commit: Fix is_read for block_job_error_action()

[Ján Tomko]

On Fri, Feb 14, 2020 at 09:08:10PM +0100, Kevin Wolf wrote:

block_job_error_action() needs to know if reading from the top node or
writing to the base node failed so that it can set the right 'operation'
in the BLOCK_JOB_ERROR QMP event.

Signed-off-by: Kevin Wolf 
---
block/commit.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs

[Ján Tomko]

On Fri, Feb 14, 2020 at 09:08:06PM +0100, Kevin Wolf wrote:

It is not obvious what 'ignore' actually means for block jobs: It could
be continuing the job and returning success in the end despite the error
(no block job does this). It could also mean continuing and returning
failure in the end (this is what stream does). And it can mean retrying
the failed request later (this is what backup, commit and mirror do).

This (somewhat inconsistent) behaviour was introduced and described for
stream and mirror in commit ae586d6158. backup and commit were


fatal: ambiguous argument 'ae586d6158': unknown revision or path not in the 
working tree.


introduced later and use the same model as mirror.

Signed-off-by: Kevin Wolf 
---
qapi/block-core.json | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 2/7] commit: Remove unused bytes_written

[Ján Tomko]

On Fri, Feb 14, 2020 at 09:08:07PM +0100, Kevin Wolf wrote:

The bytes_written variable is only ever written to, it serves no
purpose. This has actually been the case since the commit job was first
introduced in commit 747ff602636.

Signed-off-by: Kevin Wolf 
---
block/commit.c | 2 --
1 file changed, 2 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 3/7] commit: Fix argument order for block_job_error_action()

[Ján Tomko]

On Fri, Feb 14, 2020 at 09:08:08PM +0100, Kevin Wolf wrote:

The block_job_error_action() error call in the commit job gives the
on_err and is_read arguments in the wrong order. Fix this.

(Of course, hard-coded is_read = false is wrong, too, but that's a
separate problem for a separate patch.)

Signed-off-by: Kevin Wolf 
---
block/commit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 3/3] tools/virtiofsd/fuse_lowlevel: Fix fuse_out_header.error value

[Ján Tomko]

On Sat, Feb 15, 2020 at 05:07:16PM +0100, Philippe Mathieu-Daudé wrote:

Fix warning reported by Clang static code analyzer:

   CC  tools/virtiofsd/fuse_lowlevel.o
 tools/virtiofsd/fuse_lowlevel.c:195:9: warning: Value stored to 'error' is 
never read
 error = -ERANGE;
 ^   ~~~

Fixes: 2de121f01e
Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
RFC because untested
---
tools/virtiofsd/fuse_lowlevel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 2/3] tools/virtiofsd/passthrough_ll: Remove unneeded variable assignment

[Ján Tomko]

On Sat, Feb 15, 2020 at 05:07:15PM +0100, Philippe Mathieu-Daudé wrote:

Fix warning reported by Clang static code analyzer:

   CC  tools/virtiofsd/passthrough_ll.o
 tools/virtiofsd/passthrough_ll.c:925:9: warning: Value stored to 'newfd' is 
never read
 newfd = -1;
 ^   ~~
 tools/virtiofsd/passthrough_ll.c:942:9: warning: Value stored to 'newfd' is 
never read
 newfd = -1;
 ^   ~~

Fixes: 7c6b66027
Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
tools/virtiofsd/passthrough_ll.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e9e71d5fc2..b38e0e4d84 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -922,7 +922,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, 
const char *name,
inode = lo_find(lo, >attr);
if (inode) {
close(newfd);
-newfd = -1;
} else {
inode = calloc(1, sizeof(struct lo_inode));
if (!inode) {
@@ -938,8 +937,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, 
const char *name,
g_atomic_int_set(>refcount, 2);

inode->nlookup = 1;
-inode->fd = newfd;
-newfd = -1;
+inode->fd = -1;


The functional equivalent is:
inode->fd = newfd;

newfd cannot contain -1 here, as checked a few lines above:
 newfd = openat(dir->fd, name, O_PATH | O_NOFOLLOW);
 if (newfd == -1) {
 goto out_err;
 }

Jano


inode->key.ino = e->attr.st_ino;
inode->key.dev = e->attr.st_dev;
pthread_mutex_init(>plock_mutex, NULL);
--
2.21.1




signature.asc
Description: PGP signature

Re: [PATCH 1/3] tools/virtiofsd/passthrough_ll: Remove unneeded variable assignment

[Ján Tomko]

On Sat, Feb 15, 2020 at 05:07:14PM +0100, Philippe Mathieu-Daudé wrote:

Fix warning reported by Clang static code analyzer:

   CC  tools/virtiofsd/passthrough_ll.o
 tools/virtiofsd/passthrough_ll.c:1083:5: warning: Value stored to 'saverr' is 
never read
 saverr = ENOMEM;
 ^~~

Fixes: 7c6b66027
Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
tools/virtiofsd/passthrough_ll.c | 2 --
1 file changed, 2 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 1/3] block/qcow2-bitmap: Remove unneeded variable assignment

[Ján Tomko]

On Sat, Feb 15, 2020 at 05:15:55PM +0100, Philippe Mathieu-Daudé wrote:

Fix warning reported by Clang static code analyzer:

   CC  block/qcow2-bitmap.o
 block/qcow2-bitmap.c:650:5: warning: Value stored to 'ret' is never read
 ret = -EINVAL;
 ^ ~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
block/qcow2-bitmap.c | 1 -
1 file changed, 1 deletion(-)



Reviewed-by: Ján Tomko 

Unused since its introduction in 88ddffae8fc1e30cc907c2dbb989b7eba9e62319

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2] Report stringified errno in VFIO related errors

[Ján Tomko]

On Fri, Feb 14, 2020 at 10:55:19AM +0100, Michal Privoznik wrote:

In a few places we report errno formatted as a negative integer.
This is not as user friendly as it can be. Use strerror() and/or
error_setg_errno() instead.

Signed-off-by: Michal Privoznik 
---

v1 posted here:

https://lists.nongnu.org/archive/html/qemu-devel/2020-02/msg03623.html

diff to v1:
- Change error reporting in vfio_dma_unmap() too as I missed it in v1.

hw/vfio/common.c| 4 ++--
util/vfio-helpers.c | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] Report stringified errno in VFIO related errors

[Ján Tomko]

On Fri, Feb 14, 2020 at 09:47:39AM +0100, Michal Privoznik wrote:

In a few places we report errno formatted as a negative integer.
This is not as user friendly as it can be. Use strerror() and/or
error_setg_errno() instead.

Signed-off-by: Michal Privoznik 
---
hw/vfio/common.c| 2 +-
util/vfio-helpers.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] nbd-client: Support leading / in NBD URI

[Ján Tomko]

On Tue, Feb 11, 2020 at 08:31:01PM -0600, Eric Blake wrote:

The NBD URI specification [1] states that only one leading slash at
the beginning of the URI path component is stripped, not all such
slashes.  This becomes important to a patch I just proposed to nbdkit
[2], which would allow the exportname to select a file embedded within
an ext2 image: ext2fs demands an absolute pathname beginning with '/',
and because qemu was inadvertantly stripping it, my nbdkit patch had
to work around the behavior.

[1] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md
[2] https://www.redhat.com/archives/libguestfs/2020-February/msg00109.html

Note that the qemu bug only affects handling of URIs such as
nbd://host:port//abs/path (where '/abs/path' should be the export
name); it is still possible to use --image-opts and pass the desired
export name with a leading slash directly through JSON even without
this patch.

Signed-off-by: Eric Blake 
---
block/nbd.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] qapi: Expand documentation for LostTickPolicy

[Ján Tomko]

On Tue, Feb 11, 2020 at 07:37:44PM +0100, Andrea Bolognani wrote:

The current documentation is fairly terse and not easy to decode
for someone who's not intimately familiar with the inner workings
of timer devices. Expand on it by providing a somewhat verbose


Perchance exorbitantly circumlocutory, but definitely an improvement.


description of what behavior each policy will result in, as seen
from both the guest OS and host point of view.

Signed-off-by: Andrea Bolognani 
---
This information is reported pretty much word by word in

 https://libvirt.org/formatdomain.html#elementsTime

so I'm hoping I can get the QEMU documentation updated and then just
merge back the changes.

qapi/misc.json | 34 +++---
1 file changed, 23 insertions(+), 11 deletions(-)


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v3 0/7] ui: rework -show-cursor option

[Ján Tomko]

On Fri, Feb 07, 2020 at 11:17:46AM +0100, Gerd Hoffmann wrote:

Add -display {sdl,gtk,cocoa},show-cursor=on as replacement for
-show-cursor.  sdl + cocoa are switched over (no change in behavior),
gtk support is added.

Gerd Hoffmann (7):
 ui: add show-cursor option
 ui: wire up legacy -show-cursor option
 ui/sdl: switch to new show-cursor option
 ui/cocoa: switch to new show-cursor option
 ui/gtk: implement show-cursor option
 ui: drop curor_hide global variable.
 ui: deprecate legacy -show-cursor option

include/sysemu/sysemu.h |  1 -
ui/gtk.c|  8 ++--
ui/sdl2.c   | 16 
vl.c| 16 ++--
qapi/ui.json|  3 +++
qemu-deprecated.texi|  5 +
ui/cocoa.m  |  4 
7 files changed, 40 insertions(+), 13 deletions(-)



Series:
Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 2/5] ui/gtk: implement show-cursor option

[Ján Tomko]

On Wed, Feb 05, 2020 at 12:03:53PM +0100, Gerd Hoffmann wrote:

Signed-off-by: Gerd Hoffmann 
---
ui/gtk.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 3/5] ui/sdl: implement show-cursor option

[Ján Tomko]

On Wed, Feb 05, 2020 at 12:03:54PM +0100, Gerd Hoffmann wrote:

Signed-off-by: Gerd Hoffmann 
---
ui/sdl2.c | 28 
1 file changed, 20 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 1/5] ui: add show-cursor option

[Ján Tomko]

On Wed, Feb 05, 2020 at 12:03:52PM +0100, Gerd Hoffmann wrote:

When enabled forces showing a the mouse cursor, i.e. do
nowallow the guest to hide it.  Defaults to off.

Signed-off-by: Gerd Hoffmann 
---
qapi/ui.json | 2 ++
1 file changed, 2 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 4/5] ui: wire up legacy -show-cursor option

[Ján Tomko]

On Wed, Feb 05, 2020 at 12:03:55PM +0100, Gerd Hoffmann wrote:

Signed-off-by: Gerd Hoffmann 
---
include/sysemu/sysemu.h | 1 -
vl.c| 4 ++--
2 files changed, 2 insertions(+), 3 deletions(-)




diff --git a/vl.c b/vl.c
index 24951b51a94b..0db0aa0fa040 100644
--- a/vl.c
+++ b/vl.c
@@ -3553,7 +3552,8 @@ int main(int argc, char **argv, char **envp)
no_shutdown = 1;
break;
case QEMU_OPTION_show_cursor:
-cursor_hide = 0;


Since commit 13aefd303cf996c2d183e94082413885bf1d15bf
cursor_hide is also used in ui/cocoa.m

Jano


+dpy.has_show_cursor = true;
+dpy.show_cursor = true;
break;
case QEMU_OPTION_uuid:
if (qemu_uuid_parse(optarg, _uuid) < 0) {
--
2.18.1




signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] qemu-nbd: Removed deprecated --partition option

[Ján Tomko]

On Thu, Jan 23, 2020 at 10:46:50AM -0600, Eric Blake wrote:

The option was deprecated in 4.0.0 (commit 0ae2d546); it's now been
long enough with no complaints to follow through with that process.

Signed-off-by: Eric Blake 
---
docs/interop/qemu-nbd.rst |  15 ++---
qemu-deprecated.texi  |  49 ++
qemu-nbd.c| 133 +-
3 files changed, 24 insertions(+), 173 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 1/2] docs: Fix typo in qemu-nbd -P replacement

[Ján Tomko]

On Thu, Jan 23, 2020 at 10:46:49AM -0600, Eric Blake wrote:

The suggested replacement for the deprecated 'qemu-nbd -P' referw to


s/referw/refer/


'file.backing.opt' instead of 'file.file.opt'; using the example
verbatim results in:

qemu-nbd: Failed to blk_new_open 
'driver=raw,offset=1m,size=100m,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file4':
 A block device must be specified for "file"

Correct this text, prior to actually finishing the deprecation process.

Fixes: 0ae2d54645eb
Reported-by: Max Reitz 
Signed-off-by: Eric Blake 
---
qemu-deprecated.texi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 5/5] ui: deprecate legacy -show-cursor option

[Ján Tomko]

On Wed, Feb 05, 2020 at 12:03:56PM +0100, Gerd Hoffmann wrote:

Signed-off-by: Gerd Hoffmann 
---
vl.c | 2 ++
qemu-deprecated.texi | 5 +
2 files changed, 7 insertions(+)



libvirt does not use -show-cursor

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

[Ján Tomko]

On Mon, Jan 27, 2020 at 04:01:31PM -0500, John Snow wrote:



On 1/27/20 3:43 PM, Peter Krempa wrote:

On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:



On 1/27/20 5:36 AM, Maxim Levitsky wrote:

This patch series is bunch of cleanups
to the hmp monitor code.

This series only touched blockdev related hmp handlers.

No functional changes expected other that
light error message changes by the last patch.

This was inspired by this bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1719169

Basically some users still parse hmp error messages,
and they would like to have them prefixed with 'Error:'





[...]


The bug was reported at the time when libvirt didn't use blockdev yet,
but at this point it's pointless from our side.


Just for the record, this bug was the motivation for the prefix request:
https://bugzilla.redhat.com/show_bug.cgi?id=1718255


This wouldn't even fix
the scenario when old (pre-5.10) libvirt would use new qemu because the
drive-add handler never checked the error prefix.


And running older libvirt with newer QEMU is not something we care
about.

Jano



[1] 
https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD



Thank you for the report from libvirtville :)

--js




signature.asc
Description: PGP signature

Re: [libvirt] [PATCH for-5.0 3/4] Remove the core bluetooth code

[Ján Tomko]

On Wed, Nov 20, 2019 at 10:10:13AM +0100, Thomas Huth wrote:

It's been deprecated since QEMU v3.1. We've explicitly asked in the
deprecation message that people should speak up on qemu-devel in case
they are still actively using the bluetooth part of QEMU, but nobody
ever replied that they are really still using it.

I've tried it on my own to use this bluetooth subsystem for one of my
guests, but I was also not able to get it running anymore: When I was
trying to pass-through a real bluetooth device, either the guest did
not see the device at all, or the guest crashed.

Even worse for the emulated device: When running

qemu-system-x86_64 -bt device:keyboard

QEMU crashes once you hit a key.

So it seems like the bluetooth stack is not only neglected, it is
completely bitrotten, as far as I can tell. The only attention that
this code got during the past years were some CVEs that have been
spotted there. So this code is a burden for the developers, without
any real benefit anymore. Time to remove it.

Signed-off-by: Thomas Huth 
---
Makefile.objs|2 -
bt-host.c|  198 
bt-vhci.c|  167 
configure|   31 -
hw/Kconfig   |1 -
hw/Makefile.objs |1 -
hw/bt/Kconfig|2 -
hw/bt/Makefile.objs  |3 -
hw/bt/core.c |  143 ---
hw/bt/hci-csr.c  |  512 --
hw/bt/hci.c  | 2263 --
hw/bt/hid.c  |  553 ---
hw/bt/l2cap.c| 1367 -
hw/bt/sdp.c  |  989 --
include/hw/bt.h  | 2177 
include/sysemu/bt.h  |   20 -
qemu-deprecated.texi |7 -
qemu-options.hx  |   79 --
vl.c |  136 ---
19 files changed, 8651 deletions(-)


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt] [PATCH for-5.0 2/4] hw/usb: Remove the USB bluetooth dongle device

[Ján Tomko]

On Wed, Nov 20, 2019 at 10:10:12AM +0100, Thomas Huth wrote:

We are going to remove the bluetooth backend, so the USB bluetooth
dongle can not work anymore. It's a completely optional device, no
board depends on it, so let's simply remove it now.

Signed-off-by: Thomas Huth 
---
hw/usb/Kconfig |   5 -
hw/usb/Makefile.objs   |   1 -
hw/usb/dev-bluetooth.c | 581 -
qemu-doc.texi  |  15 --
4 files changed, 602 deletions(-)
delete mode 100644 hw/usb/dev-bluetooth.c



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [Qemu-devel] [libvirt] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices

[Ján Tomko]

On Wed, Mar 06, 2019 at 08:41:48AM +0100, Peter Krempa wrote:

On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:

On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:


[...]


So I agree neither scenario is exactly perfect, but I still think
adding non-transitional alias devices would overall be more
user-friendly.


I don't think it makes sense to add it at the qemu level. From libvirt's
point of view users should be shielded from any qemu impl detail or
inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
devices would be the same and thus does not make sense to do that
because it would be more confusing.

You can argue that we should add the alias at the libvirt level though.



You can, but please don't.

Jano


[1] Yes. I'm aware that statement is quite ironical.





--
libvir-list mailing list
libvir-l...@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list




signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2] vhost-user: define conventions for vhost-user backends

[Ján Tomko]

On Fri, Sep 14, 2018 at 05:52:30PM +0400, Marc-André Lureau wrote:

As discussed during "[PATCH v4 00/29] vhost-user for input & GPU"
review, let's define a common set of backend conventions to help with
management layer implementation, and interoperability.

v2:
- drop --pidfile
- add some notes about daemonizing & stdin/out/err

Cc: libvir-l...@redhat.com
Cc: Gerd Hoffmann 
Cc: Daniel P. Berrangé 
Cc: Changpeng Liu 
Cc: Dr. David Alan Gilbert 
Cc: Felipe Franciosi 
Cc: Gonglei 
Cc: Maxime Coquelin 
Cc: Michael S. Tsirkin 
Cc: Victor Kaplansky 
Signed-off-by: Marc-André Lureau 
---
docs/interop/vhost-user.txt | 109 +++-
1 file changed, 107 insertions(+), 2 deletions(-)

diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
index ba5e37d714..339b335e9c 100644
--- a/docs/interop/vhost-user.txt
+++ b/docs/interop/vhost-user.txt
@@ -17,8 +17,13 @@ The protocol defines 2 sides of the communication, master 
and slave. Master is
the application that shares its virtqueues, in our case QEMU. Slave is the
consumer of the virtqueues.

-In the current implementation QEMU is the Master, and the Slave is intended to
-be a software Ethernet switch running in user space, such as Snabbswitch.
+In the current implementation QEMU is the Master, and the Slave is the
+external process consuming the virtio queues, for example a software
+Ethernet switch running in user space, such as Snabbswitch, or a block
+device backend processing read & write to a virtual disk. In order to
+facilitate interoperability between various backend implementations,
+it is recommended to follow the "Backend program conventions"
+described in this document.

Master and slave can be either a client (i.e. connecting) or server (listening)
in the socket communication.
@@ -859,3 +864,103 @@ resilient for selective requests.
For the message types that already solicit a reply from the client, the
presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
no behavioural change. (See the 'Communication' section for details.)
+
+Backend program conventions
+---
+
+vhost-user backends provide various services and they may need to be
+configured manually depending on the use case. However, it is a good
+idea to follow the conventions listed here when possible. Users, QEMU
+or libvirt, can then rely on some common behaviour to avoid
+heterogenous configuration and management of the backend program and
+facilitate interoperability.
+
+In order to be discoverable, default vhost-user backends should be
+located under "/usr/libexec", and be named "vhost-user-$device" where
+"$device" is the device name in lower-case following the name listed
+in the Linux virtio_ids.h header (ex: the VIRTIO_ID_RPROC_SERIAL
+backend would be named "vhost-user-rproc-serial").
+
+Mechanisms to list, and to select among alternatives implementations
+or modify the default backend are not described at this point (a
+distribution may use update-alternatives, for example, to list and to
+pick a different default backend).
+
+The backend program must not daemonize itself, but it may be
+daemonized by the management layer. It may also have a restricted
+access to the system.
+
+File descriptors 0, 1 and 2 will exist, and have regular
+stdin/stdout/stderr usage (they may be redirected to /dev/null by the
+management layer, or to a log handler).
+
+The backend program must end (as quickly and cleanly as possible) when
+the SIGTERM signal is received. Eventually, it may be SIGKILL by the
+management layer after a few seconds.
+
+The following command line options have an expected behaviour. They
+are mandatory, unless explicitly said differently:
+
+* --socket-path=PATH
+
+This option specify the location of the vhost-user Unix domain socket.


Will the backend program listen on this socket or connect to it?


+It is incompatible with --fd.
+
+* --fd=FDNUM
+
+When this argument is given, the backend program is started with the
+vhost-user socket as file descriptor FDNUM. It is incompatible with
+--socket-path.
+
+* --print-capabilities
+
+Output to stdout a line-seperated list of backend capabilities, and
+then exit successfully. Other options and arguments should be ignored,
+and the backend program should not perform its normal function.
+
+At the time of writing, there are no common capabilities. Some
+device-specific capabilities are listed in the respective sections. By
+convention, device-specific capabilities are prefixed by their device
+name.


Ideally, this would not be needed by libvirt.
QEMU capability probing is nice to have when we need to provide a
helpful error message (and avoid the lengthy preparation and cleanup
in case it fails), but absolutely necessary if we need to supply
different command line arguments to maintain the same functionality.

I was really happy to be able to delete qemu-img help parsing when
we dropped support for QEMU < 1.5.0 in upstream libvirt,
see commit 

Re: [Qemu-devel] [libvirt] [PATCH 0/4] Remove more deprecated options

[Ján Tomko]

On Mon, Aug 27, 2018 at 05:08:45PM +0200, Peter Krempa wrote:

On Mon, Aug 27, 2018 at 16:54:08 +0200, Thomas Huth wrote:

These options are deprecated since at least two releases, and nobody
complained. Time to remove them now.

(I'm sending these patches as a series since Paolo asked me to send a PULL
request on my own for them ... but if one of the subsystems maintainers
prefers to take the patches through their own tree instead, just let me
know)

Thomas Huth (4):
  Remove the deprecated -balloon option
  Remove the deprecated -nodefconfig option
  Remove the deprecated options -startdate, -localtime and -rtc-td-hack
  net: Remove the deprecated -tftp, -bootp, -redir and -smb options


Libvirt does not use any of the options above thankfully so a weak-ACK
from the libvirt side.


And another:
ACK yeah!
from the libvirt side.

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [libvirt] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net

[Ján Tomko]

On Wed, Jun 06, 2018 at 11:17:36AM -0700, Samudrala, Sridhar wrote:

On 6/4/2018 7:06 PM, Jason Wang wrote:



On 2018年06月05日 09:41, Samudrala, Sridhar wrote:

Ping on this patch now that the kernel patches are accepted into
davem's net-next tree.
https://patchwork.ozlabs.org/cover/920005/


On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:

This feature bit can be used by hypervisor to indicate virtio_net
device to
act as a standby for another device with the same MAC address.

I tested this with a small change to the patch to mark the STANDBY
feature 'true'
by default as i am using libvirt to start the VMs.
Is there a way to pass the newly added feature bit 'standby' to qemu
via libvirt
XML file?



Maybe you can try qemu command line passthrough:

https://libvirt.org/drvqemu.html#qemucommand


It looks like this can be used to pass command line arguments to qemu.
Is it possible to specify a virtio specific attribute via this method?



Yes, for testing purposes you should be able to do this via using QEMU's
-set command line argument:
http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html
i.e.:


 ...
 
   
   
 




For ex: to say mrg_rxbuf is off we can add the following line to virtio
section of the domain xml file.
  

I think libvirt needs to be extended to to support the new 'standby' attribute
via this mechanism.
Adding Liane Stump and libvirt to the CC list.


*Laine



Michael,
Can we start with getting this patch into Qemu and an update to libvirt to
support the 'standby' feature so that this feature can be enabled via
some scripts/orchestration layer for now.

We could improve this solution by enhancing Qemu to do automatic management of 
the
addition/deletion of the primary device based on feature negotiation as a later 
patch.



If that means the libvirt attribute would no longer be needed, I don't
see the reason to add it to libvirt in the first place.

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined

[Ján Tomko]

On Tue, May 29, 2018 at 03:31:40PM +0800, Yi Min Zhao wrote:

If CONFIG_SECCOMP is undefined, the option 'elevateprivileges' remains
compiled. This would make libvirt set the corresponding capability and
then trigger failure during guest startup. This patch moves the code
regarding seccomp command line options to qemu-seccomp.c file and
wraps qemu_opts_foreach finding sandbox option with CONFIG_SECCOMP.
Because parse_sandbox() is moved into qemu-seccomp.c file, change
seccomp_start() to static function.

Signed-off-by: Yi Min Zhao 
---
1. Problem Description
==
If QEMU is built without seccomp support, 'elevateprivileges' remains compiled.
This option of sandbox is treated as an indication for seccomp blacklist support
in libvirt. This behavior is introduced by the libvirt commits 31ca6a5 and
3527f9d. It would make libvirt build wrong QEMU cmdline, and then the guest
startup would fail.

2. Libvirt Log
==
qemu-system-s390x: -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
resourcecontrol=deny: seccomp support is disabled

3. Fixup

Move the code related ot sandbox to qemu-seccomp.c file and wrap them with
CONFIG_SECCOMP. So compile the code related to sandbox only when
CONFIG_SECCOMP is defined.
---
include/sysemu/seccomp.h |   3 +-
qemu-seccomp.c   | 121 ++-
vl.c | 118 +
3 files changed, 124 insertions(+), 118 deletions(-)



Reviewed-by: Ján Tomko 
Tested-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH] configure: print virglrenderer version

[Ján Tomko]

On Fri, May 25, 2018 at 05:36:09PM +0200, Marc-André Lureau wrote:

Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
---
configure | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko <jto...@redhat.com>

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH] bochs-display: add missing break

[Ján Tomko]

On Fri, May 25, 2018 at 06:53:44AM +0200, Gerd Hoffmann wrote:

Fixes: CID 1391291
Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
---
hw/display/bochs-display.c | 1 +
1 file changed, 1 insertion(+)



Reviewed-by: Ján Tomko <jto...@redhat.com>

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH] migration: use g_free for ram load bitmap

[Ján Tomko]

On Fri, May 25, 2018 at 09:50:42AM +0800, Peter Xu wrote:

Buffers allocated with bitmap_new() should be freed with g_free().

Both reported by Coverity:

*** CID 1391300:  API usage errors  (ALLOC_FREE_MISMATCH)
/migration/ram.c: 3517 in ram_dirty_bitmap_reload()
3511  * the last one to sync, we need to notify the main send thread.
3512  */
3513 ram_dirty_bitmap_reload_notify(s);
3514
3515 ret = 0;
3516 out:

CID 1391300:  API usage errors  (ALLOC_FREE_MISMATCH)
Calling "free" frees "le_bitmap" using "free" but it should have been freed using 
"g_free".

3517 free(le_bitmap);
3518 return ret;
3519 }
3520
3521 static int ram_resume_prepare(MigrationState *s, void *opaque)
3522 {

*** CID 1391292:  API usage errors  (ALLOC_FREE_MISMATCH)
/migration/ram.c: 249 in ramblock_recv_bitmap_send()
243  * Mark as an end, in case the middle part is screwed up due to
244  * some "misterious" reason.
245  */
246 qemu_put_be64(file, RAMBLOCK_RECV_BITMAP_ENDING);
247 qemu_fflush(file);
248

CID 1391292:  API usage errors  (ALLOC_FREE_MISMATCH)
Calling "free" frees "le_bitmap" using "free" but it should have been freed using 
"g_free".

249 free(le_bitmap);
250
251 if (qemu_file_get_error(file)) {
252 return qemu_file_get_error(file);
253 }
254

Signed-off-by: Peter Xu <pet...@redhat.com>
---
migration/ram.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)



Reviewed-by: Ján Tomko <jto...@redhat.com>

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH v2 1/1] sandbox: disable -sandbox if CONFIG_SECCOMP undefined

[Ján Tomko]

On Sat, May 19, 2018 at 04:20:37PM +0800, Yi Min Zhao wrote:



在 2018/5/18 下午9:07, Ján Tomko 写道:

On Fri, May 18, 2018 at 11:19:16AM +0200, Eduardo Otubo wrote:

On 18/05/2018 - 09:52:12, Ján Tomko wrote:

But now libvirt requires QEMU >= 1.5.0 which already supports
query-command-line-options, so if you want the option gone completely
--without-seccomp, I can add the code that probes for it and
make seccomp_sandbox = 0 a no-op if it's compiled out.


This looks like a good solution for the libvirt side. Can you add
this support
so we can merge this fix?



Patches proposed:
https://www.redhat.com/archives/libvir-list/2018-May/msg01430.html

Jano

Thanks for your work!


Now pushed in libvirt master:
commit b87222a90919040c12fb6d7c8dcc20f944a66495
Author:     Ján Tomko <jto...@redhat.com>
AuthorDate: 2018-05-18 14:57:51 +0200
Commit: Ján Tomko <jto...@redhat.com>
CommitDate: 2018-05-23 09:45:48 +0200

   qemu: only pass -sandbox off if supported

   This way we don't rely on QEMU supplying the -sandbox option
   without CONFIG_SECCOMP.

   Signed-off-by: Ján Tomko <jto...@redhat.com>
   Reviewed-by: John Ferlan <jfer...@redhat.com>

git describe: v4.3.0-258-gb87222a909
https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=b87222a90919040c12fb6d7c8dcc20f944a66495

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH v2 1/1] sandbox: disable -sandbox if CONFIG_SECCOMP undefined

[Ján Tomko]

On Fri, May 18, 2018 at 11:19:16AM +0200, Eduardo Otubo wrote:

On 18/05/2018 - 09:52:12, Ján Tomko wrote:

On Thu, May 17, 2018 at 02:41:09PM +0200, Eduardo Otubo wrote:
> On 15/05/2018 - 19:33:48, Yi Min Zhao wrote:
> > If CONFIG_SECCOMP is undefined, the option 'elevateprivileges' remains
> > compiled. This would make libvirt set the corresponding capability and
> > then trigger the guest startup fails. So this patch excludes the code
> > regarding seccomp staff if CONFIG_SECCOMP is undefined.
>
> Just a sugestion for the next patch you send: If it's a single patch, you 
don't
> need to format it with a cover-letter. Just put all the description in the 
body,
> or if you need to add a text that shouldn't be included in the commit message,
> just add it after the "---" after Signed-off-by.
>
> >
> > Signed-off-by: Yi Min Zhao <zyi...@linux.ibm.com>
> > ---
> >  vl.c | 13 -
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >



[...]


Current libvirt logic assumes the -sandbox option is always present.
(IIRC it was introduced in QEMU 1.1 and when we switched from help
scraping to capability probing via QMP for QEMU 1.2, there was no
way to detect it)

This patch fixes the usage of QEMU new enough for seccomp blacklist
(where libvirt enables the sandbox by default),
but breaks the usage of QEMU with compiled out sandbox and
setting
 seccomp_sandbox = 0
in libvirt's qemu.conf:

error: internal error: process exited while connecting to monitor:
qemu-git: -sandbox off: There is no option group 'sandbox'


But now libvirt requires QEMU >= 1.5.0 which already supports
query-command-line-options, so if you want the option gone completely
--without-seccomp, I can add the code that probes for it and
make seccomp_sandbox = 0 a no-op if it's compiled out.


This looks like a good solution for the libvirt side. Can you add this support
so we can merge this fix?



Patches proposed:
https://www.redhat.com/archives/libvir-list/2018-May/msg01430.html

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH v2 1/1] sandbox: disable -sandbox if CONFIG_SECCOMP undefined

[Ján Tomko]

On Thu, May 17, 2018 at 02:41:09PM +0200, Eduardo Otubo wrote:

On 15/05/2018 - 19:33:48, Yi Min Zhao wrote:

If CONFIG_SECCOMP is undefined, the option 'elevateprivileges' remains
compiled. This would make libvirt set the corresponding capability and
then trigger the guest startup fails. So this patch excludes the code
regarding seccomp staff if CONFIG_SECCOMP is undefined.


Just a sugestion for the next patch you send: If it's a single patch, you don't
need to format it with a cover-letter. Just put all the description in the body,
or if you need to add a text that shouldn't be included in the commit message,
just add it after the "---" after Signed-off-by.



Signed-off-by: Yi Min Zhao 
---
 vl.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)




@@ -4071,10 +4072,12 @@ int main(int argc, char **argv, char **envp)
 exit(1);
 }

+#ifdef CONFIG_SECCOMP
 if (qemu_opts_foreach(qemu_find_opts("sandbox"),
   parse_sandbox, NULL, NULL)) {
 exit(1);
 }
+#endif

 if (qemu_opts_foreach(qemu_find_opts("name"),
   parse_name, NULL, NULL)) {
--
Yi Min



I just wanted a review from Ján, since he is the author of the original libvirt
patch. Does this breaks libvirt logic in any way? If not, ACK on this patch.



Current libvirt logic assumes the -sandbox option is always present.
(IIRC it was introduced in QEMU 1.1 and when we switched from help
scraping to capability probing via QMP for QEMU 1.2, there was no
way to detect it)

This patch fixes the usage of QEMU new enough for seccomp blacklist
(where libvirt enables the sandbox by default),
but breaks the usage of QEMU with compiled out sandbox and
setting
 seccomp_sandbox = 0
in libvirt's qemu.conf:

error: internal error: process exited while connecting to monitor:
qemu-git: -sandbox off: There is no option group 'sandbox'


But now libvirt requires QEMU >= 1.5.0 which already supports
query-command-line-options, so if you want the option gone completely
--without-seccomp, I can add the code that probes for it and
make seccomp_sandbox = 0 a no-op if it's compiled out.

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined

[Ján Tomko]

On Mon, May 07, 2018 at 01:04:17PM -0500, Eric Blake wrote:

On 05/06/2018 10:32 PM, Yi Min Zhao wrote:

In the subject line: s/avoid to compile/avoid compiling/


If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains


s/elevator/elevate/


complied. This would make libvirt set the corresponding capability and


s/complied/compiled/


then trigger the guest startup fails. So let's wrap the options with
CONFIG_SECCOMP.

Signed-off-by: Yi Min Zhao 
---
  vl.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/vl.c b/vl.c
index fce1fd12d8..cb07b19c02 100644
--- a/vl.c
+++ b/vl.c
@@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
  .name = "enable",
  .type = QEMU_OPT_BOOL,
  },
+#ifdef CONFIG_SECCOMP
  {
  .name = "obsolete",
  .type = QEMU_OPT_STRING,
@@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
  .name = "resourcecontrol",
  .type = QEMU_OPT_STRING,
  },
+#endif


The commit message mentions only 'elevateprivileges' (once the typo is
fixed), but you are also crippling 'obsolete', 'spawn', and
'resourcecontrol'.  Perhaps the commit message should call that out
better?  Or, since libvirt is looking at just 'elevateprivileges', per
this line in libvirt's qemu_capabilities.c:

src/qemu/qemu_capabilities.c:{ "sandbox", "elevateprivileges",
QEMU_CAPS_SECCOMP_BLACKLIST },

is it sufficient to just mask out that one option?


That would be inconsistent. I picked one option randomly, because they
were added at the same time, but compiling out just one out of four
is just odd. And with leaving them in even though the functionality is
compiled out, libvirt has no way to tell upfront whether it's usable.

By that logic, removing the -sandbox option without CONFIG_SECCOMP makes
sense, but libvirt already assumes this option is present on all supported
QEMUs (>= 1.5.0), so please cc: me on that change if you decide to
remove it as well.

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [libvirt] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest

[Ján Tomko]

On Mon, May 07, 2018 at 12:33:20PM +0200, Eduardo Otubo wrote:

On 07/05/2018 - 11:29:57, Christian Borntraeger wrote:

On 05/07/2018 05:32 AM, Yi Min Zhao wrote:
> 1. Problem Description
> ==
> If QEMU is built without seccomp support, 'elevatorprivileges' remains 
compiled.
> This option of sandbox is treated as an indication for seccomp blacklist 
support
> in libvirt. This behavior is introduced by the libvirt commits 31ca6a5 and
> 3527f9d. It would make libvirt build wrong QEMU cmdline, and then the guest
> startup would fail.

Adding libvirt list.

This would still fail with older QEMUs, so the question is if we should also OR 
instead
change something in libvirt.


Perhaps I'm missing something here, but libvirt can differentiate between
different versions of QEMU, therefore not calling it with wrong or outdated
arguments.



The code introduced in libvirt commit 31ca6a5 specifically looks for
'elevateprivileges' in 'parameters' of the 'sandbox' option through
query-command-line-options.

Outdated QEMUs should not have this option there.

However, libvirtd does add the option by default not knowing whether it
can fail for other reasons, e.g. SECCOMP not being enabled in the
running kernel. I wonder if that is worth addressing.

Jano



>
> 2. Libvirt Log
> ==
> qemu-system-s390x: -sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
> resourcecontrol=deny: seccomp support is disabled
>
> 3. Fixup
> 
> Wrap the options except 'enable' for qemu_sandbox_opts by CONFIG_SECCOMP.
>
> Yi Min Zhao (1):
>   sandbox: avoid to compile options if CONFIG_SECCOMP undefined
>
>  vl.c | 2 ++
>  1 file changed, 2 insertions(+)
>



--
Eduardo Otubo

--
libvir-list mailing list
libvir-l...@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features

[Ján Tomko]

On Fri, Oct 14, 2016 at 04:28:14PM -0300, Eduardo Habkost wrote:

When explicitly enabling unmigratable flags using "-cpu host"
(e.g. "-cpu host,+invtsc"), the requested feature won't be
enabled because cpu->migratable is true by default.



[...]



Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
target-i386/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Tested-by: Ján Tomko <jto...@redhat.com>


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PULL 14/19] log: Redirect stderr to logfile if deamonized

[Ján Tomko]

On Mon, Feb 29, 2016 at 11:17:25AM +0100, Paolo Bonzini wrote:
> 
> 
> On 29/02/2016 11:04, Ján Tomko wrote:
> > On Mon, Feb 29, 2016 at 10:07:47AM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 29/02/2016 09:57, Ján Tomko wrote:
> >>> qemu-system-x86_64 -S -no-user-config -nodefaults -nographic -M none
> >>>  -qmp unix:/var/lib/libvirt/qemu/capabilities.monitor.sock,server,nowait
> >>>  -pidfile /var/lib/libvirt/qemu/capabilities.pidfile -daemonize
> >>
> >> Can you test this patch?
> >>
> >> diff --git a/util/log.c b/util/log.c
> >> index 4dff00b..9402880 100644
> >> --- a/util/log.c
> >> +++ b/util/log.c
> >> @@ -56,7 +56,8 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
> >>  #ifdef CONFIG_TRACE_LOG
> >>  qemu_loglevel |= LOG_TRACE;
> >>  #endif
> >> -if ((qemu_loglevel || is_daemonized()) && !qemu_logfile) {
> >> +if (!qemu_logfile &&
> >> +(qemu_loglevel || (is_daemonized() && logfilename)) {
> >>  if (logfilename) {
> >>  qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
> >>  if (!qemu_logfile) {
> >>
> >> Thanks,
> > 
> > Still hangs,
> > 
> > (gdb) list
> > 54  {
> > 55  qemu_loglevel = log_flags;
> > 56  #ifdef CONFIG_TRACE_LOG
> > 57  qemu_loglevel |= LOG_TRACE;
> > 58  #endif
> > 59  sleep(10);
> > 60  if (!qemu_logfile &&
> > 61  (qemu_loglevel || (is_daemonized() && logfilename))) {
> > 62  if (logfilename) {
> > 63  qemu_logfile = fopen(logfilename, log_append ? "a" : 
> > "w");
> > (gdb) p qemu_logfile
> > $1 = (FILE *) 0x0
> > (gdb) p qemu_loglevel
> > $2 = 32768
> > (gdb) p logfilename
> > $3 = 0x0
> > (gdb) call is_daemonized()
> > $4 = true
> 
> Second attempt:
> 
> diff --git a/util/log.c b/util/log.c
> index 4dff00b..1530a60 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -56,7 +56,8 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
>  #ifdef CONFIG_TRACE_LOG
>  qemu_loglevel |= LOG_TRACE;
>  #endif
> -if ((qemu_loglevel || is_daemonized()) && !qemu_logfile) {
> +if (!qemu_logfile &&
> +(is_daemonized() ? logfilename : qemu_loglevel)) {
>  if (logfilename) {
>  qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
>  if (!qemu_logfile) {
> @@ -72,6 +73,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
>  }
>  } else {
>  /* Default to stderr if no log file specified */
> +assert(!is_daemonized());
>  qemu_logfile = stderr;
>  }
>  /* must avoid mmap() usage of glibc by setting a buffer "by hand" */
> 
> 

This works for me, but produces a warning:
  CCutil/log.o
util/log.c: In function ‘do_qemu_set_log’:
util/log.c:60:40: error: pointer/integer type mismatch in conditional 
expression [-Werror]
 (is_daemonized() ? logfilename : qemu_loglevel)) {

Jan

> Paolo

Re: [Qemu-devel] [PULL 14/19] log: Redirect stderr to logfile if deamonized

[Ján Tomko]

On Mon, Feb 29, 2016 at 10:07:47AM +0100, Paolo Bonzini wrote:
> 
> 
> On 29/02/2016 09:57, Ján Tomko wrote:
> > qemu-system-x86_64 -S -no-user-config -nodefaults -nographic -M none
> >  -qmp unix:/var/lib/libvirt/qemu/capabilities.monitor.sock,server,nowait
> >  -pidfile /var/lib/libvirt/qemu/capabilities.pidfile -daemonize
> 
> Can you test this patch?
> 
> diff --git a/util/log.c b/util/log.c
> index 4dff00b..9402880 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -56,7 +56,8 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
>  #ifdef CONFIG_TRACE_LOG
>  qemu_loglevel |= LOG_TRACE;
>  #endif
> -if ((qemu_loglevel || is_daemonized()) && !qemu_logfile) {
> +if (!qemu_logfile &&
> +(qemu_loglevel || (is_daemonized() && logfilename)) {
>  if (logfilename) {
>  qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
>  if (!qemu_logfile) {
> 
> Thanks,

Still hangs,

(gdb) list
54  {
55  qemu_loglevel = log_flags;
56  #ifdef CONFIG_TRACE_LOG
57  qemu_loglevel |= LOG_TRACE;
58  #endif
59  sleep(10);
60  if (!qemu_logfile &&
61  (qemu_loglevel || (is_daemonized() && logfilename))) {
62  if (logfilename) {
63  qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
(gdb) p qemu_logfile
$1 = (FILE *) 0x0
(gdb) p qemu_loglevel
$2 = 32768
(gdb) p logfilename
$3 = 0x0
(gdb) call is_daemonized()
$4 = true


If I add this on top of the above patch:
-#ifdef CONFIG_TRACE_LOG
+#ifndef CONFIG_TRACE_LOG

capability probing works and I can successfully start a domain.

Jan

Re: [Qemu-devel] [PULL 14/19] log: Redirect stderr to logfile if deamonized

[Ján Tomko]

On Wed, Feb 24, 2016 at 02:27:36PM +0100, Paolo Bonzini wrote:
> From: Dimitris Aragiorgis 
> 
> In case of daemonize, use the logfile passed with the -D option in
> order to redirect stderr to it instead of /dev/null.
> 
> Also remove some unused code in log.h.
> 
> Signed-off-by: Dimitris Aragiorgis 
> Message-Id: <1455795518-19205-1-git-send-email-dim...@arrikto.com>
> Signed-off-by: Paolo Bonzini 
> ---
>  include/qemu/log.h |  6 --
>  os-posix.c |  6 +-
>  util/log.c | 11 +--
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index 30817f7..dda65fd 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -94,12 +94,6 @@ static inline void qemu_log_close(void)
>  }
>  }
>  
> -/* Set up a new log file */
> -static inline void qemu_log_set_file(FILE *f)
> -{
> -qemu_logfile = f;
> -}
> -
>  /* define log items */
>  typedef struct QEMULogItem {
>  int mask;
> diff --git a/os-posix.c b/os-posix.c
> index cce62ed..92fa3ba 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -37,6 +37,7 @@
>  #include "qemu-options.h"
>  #include "qemu/rcu.h"
>  #include "qemu/error-report.h"
> +#include "qemu/log.h"
>  
>  #ifdef CONFIG_LINUX
>  #include 
> @@ -275,7 +276,10 @@ void os_setup_post(void)
>  
>  dup2(fd, 0);
>  dup2(fd, 1);
> -dup2(fd, 2);
> +/* In case -D is given do not redirect stderr to /dev/null */
> +if (!qemu_logfile) {
> +dup2(fd, 2);
> +}
> 

This hunk broke qemu usage in libvirt. When libvirtd runs qemu for
capability probing, it just hangs.

There is no -D on the command line:

qemu-system-x86_64 -S -no-user-config -nodefaults -nographic -M none
 -qmp unix:/var/lib/libvirt/qemu/capabilities.monitor.sock,server,nowait
 -pidfile /var/lib/libvirt/qemu/capabilities.pidfile -daemonize

Jan

Re: [Qemu-devel] [PATCH] docs: update the usage example of "dtrace" backend in tracing.txt

[Ján Tomko]

On Fri, Sep 11, 2015 at 02:58:50PM +0800, Lin Ma wrote:
> The usage example of dtrace is quite ancient, We have tracetool.py with
> different parameters instead of the original tracetool shell script for
> a long time, So update the old information.
> 
> Signed-off-by: Lin Ma <l...@suse.com>
> ---
>  docs/tracing.txt | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Ján Tomko <jto...@redhat.com>


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH 0/3] update CMOS for ISA-FDC with iobase=0x3f0

[Ján Tomko]

On Tue, Jun 23, 2015 at 06:58:41PM +0200, Laszlo Ersek wrote:
 This is (again) for the other pc-q35-2.4 ISA-FDC problem reported by
 Jan. Addressing comments from Markus.
 
 Jan, can you give it another try please? I realize this is getting old
 pretty quick, so don't bother if you don't want to.
 

This series also works for me.

Jan

 Cc: Jan Tomko jto...@redhat.com
 Cc: John Snow js...@redhat.com
 Cc: Markus Armbruster arm...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 
 Thanks
 Laszlo
 
 Laszlo Ersek (3):
   hw/i386/pc: factor out pc_cmos_init_floppy()
   hw/i386/pc: reflect any FDC @ ioport 0x3f0 in the CMOS
   hw/i386/pc: don't carry FDC from pc_basic_device_init() to
 pc_cmos_init()
 
  include/hw/i386/pc.h |   3 +-
  hw/i386/pc.c | 129 
 ++-
  hw/i386/pc_piix.c|   5 +-
  hw/i386/pc_q35.c |   5 +-
  4 files changed, 101 insertions(+), 41 deletions(-)
 
 -- 
 1.8.3.1
 


signature.asc
Description: Digital signature

Re: [Qemu-devel] [RFC 0/2] update CMOS for single hand-created ISA-FDC

[Ján Tomko]

On Sat, Jun 20, 2015 at 04:43:46AM +0200, Laszlo Ersek wrote:
 This is for the other pc-q35-2.4 ISA-FDC problem reported by Jan.
 
 Jan, can you give it a try pls?
 

Works for me.

I tried the following command line with a Fedora 21 guest:
-device isa-fdc,driveA=drive-fdc0-0-0
-drive file=/var/lib/libvirt/images/floppy,if=none,id=drive-fdc0-0-0,format=raw

and I could see the floppy drive inside the guest.

Thanks!

Jan

 Cc: Jan Tomko jto...@redhat.com
 Cc: John Snow js...@redhat.com
 Cc: Markus Armbruster arm...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 
 Laszlo Ersek (2):
   hw/i386/pc: factor out pc_cmos_init_floppy()
   hw/i386/pc: reflect an explicitly created, sole FDC in the CMOS
 
  hw/i386/pc.c | 113 
 ---
  1 file changed, 84 insertions(+), 29 deletions(-)
 
 -- 
 1.8.3.1
 


signature.asc
Description: Digital signature

[Qemu-devel] isa-fdc controller missing from q35 machine types

[Ján Tomko]

Hello,

commit ea96bc629cbd52be98b2967a4b4f72e91dfc3ee4
  i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted

dropped the controller for older machine types too, despite the commit
message.

It seems the logic in the merged commit does not match the original patch:
http://thread.gmane.org/gmane.comp.emulators.qemu.block/2054/focus=2056

Jan


signature.asc
Description: Digital signature

[Qemu-devel] [PATCH] Strip brackets from vnc host

[Ján Tomko]

Commit v2.2.0-1530-ge556032 vnc: switch to inet_listen_opts
bypassed the use of inet_parse in inet_listen, making literal
IPv6 addresses enclosed in brackets fail:

qemu-kvm: -vnc [::1]:0: Failed to start VNC server on `(null)': address
resolution failed for [::1]:5900: Name or service not known

Strip the brackets to make it work again.

Signed-off-by: Ján Tomko jto...@redhat.com
---
 ui/vnc.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index cffb5b7..49af7c7 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3482,7 +3482,14 @@ void vnc_display_open(const char *id, Error **errp)
 
 h = strrchr(vnc, ':');
 if (h) {
-char *host = g_strndup(vnc, h - vnc);
+char *host;
+size_t hlen = h - vnc;
+
+if (vnc[0] == '['  vnc[hlen-1] == ']') {
+host = g_strndup(vnc+1, hlen - 2);
+} else {
+host = g_strndup(vnc, hlen);
+}
 qemu_opt_set(sopts, host, host, error_abort);
 qemu_opt_set(wsopts, host, host, error_abort);
 qemu_opt_set(sopts, port, h+1, error_abort);
-- 
2.0.5

[Qemu-devel] [PATCH v2] Strip brackets from vnc host

[Ján Tomko]

Commit v2.2.0-1530-ge556032 vnc: switch to inet_listen_opts
bypassed the use of inet_parse in inet_listen, making literal
IPv6 addresses enclosed in brackets fail:

qemu-kvm: -vnc [::1]:0: Failed to start VNC server on `(null)': address
resolution failed for [::1]:5900: Name or service not known

Strip the brackets to make it work again.

Signed-off-by: Ján Tomko jto...@redhat.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
v2: Fixed spacing around operators
cc'd qemu-stable

 ui/vnc.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index cffb5b7..f989dfb 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3482,7 +3482,14 @@ void vnc_display_open(const char *id, Error **errp)
 
 h = strrchr(vnc, ':');
 if (h) {
-char *host = g_strndup(vnc, h - vnc);
+char *host;
+size_t hlen = h - vnc;
+
+if (vnc[0] == '['  vnc[hlen - 1] == ']') {
+host = g_strndup(vnc + 1, hlen - 2);
+} else {
+host = g_strndup(vnc, hlen);
+}
 qemu_opt_set(sopts, host, host, error_abort);
 qemu_opt_set(wsopts, host, host, error_abort);
 qemu_opt_set(sopts, port, h+1, error_abort);
-- 
2.0.5

Re: [Qemu-devel] [libvirt] [PATCHv2] Don't log an internal error when the guest hasn't updated balloon stats

[Ján Tomko]

On 05/15/2014 11:19 PM, Eric Blake wrote:
 On 05/15/2014 01:22 AM, Ján Tomko wrote:
 If virDomainMemoryStats is called too soon after domain startup,
 QEMU returns:
 error:{class:GenericError,desc:guest hasn't updated any stats yet}
 when we try to query balloon stats.

 Check for this reply and log it as OPERATION_INVALID instead of
 INTERNAL_ERROR. This means the daemon only logs it at the debug level,
 without polluting system logs.

 Reported by Laszlo Pal:
 https://www.redhat.com/archives/libvirt-users/2014-May/msg00023.html
 ---
 v1: https://www.redhat.com/archives/libvir-list/2014-May/msg00420.html
 v2:
   return 0 in this case - even though balloon stats are not yet available,
 we can still return 'rss' in qemuDomainMemoryStats
   jump to cleanup if CheckError returns  0

  src/qemu/qemu_monitor_json.c | 18 ++
  1 file changed, 14 insertions(+), 4 deletions(-)
 
 +if ((data = virJSONValueObjectGet(reply, error))) {
 +const char *klass = virJSONValueObjectGetString(data, class);
 +const char *desc = virJSONValueObjectGetString(data, desc);
  
 -if (ret  0)
 +if (STREQ_NULLABLE(klass, GenericError) 
 +STREQ_NULLABLE(desc, guest hasn't updated any stats yet)) {
 
 Adding qemu.  Uggh - the qemu documentation of QMP states:
 
 - The desc member is a human-readable error message. Clients should
   not attempt to parse this message.
 
 because the contents of that field are NOT guaranteed to be stable.
 We're stuck parsing that field for old versions of qemu, but this is one
 case where upstream qemu (for future versions) should change the class
 member of that particular error case to a distinct value other than
 GenericError so that it is trivially obvious when this particular
 condition has occurred, since it is a case where libvirt wants to treat
 it as a non-error.
 
 reluctant ACK, while hoping that we can do something more reliable in
 the future.
 

I have pushed the patch now.

The qemu patch reporting empty stats instead of this error should be on its way:
https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04295.html

Jan




signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2] virtio-balloon: return empty data when no stats are available

[Ján Tomko]

If the guest hasn't updated the stats yet, instead of returning
an error, return '-1' for the stats and '0' as 'last-update'.

This lets applications ignore this without parsing the error message.

Related libvirt patch and discussion:
https://www.redhat.com/archives/libvir-list/2014-May/msg00460.html

Tested against current upstream libvirt - stat reporting works and
it no longer logs errors when the stats are queried on domain startup.
(Note: libvirt doesn't use the last-update field for anything yet)

Signed-off-by: Ján Tomko jto...@redhat.com
---
v1: https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03643.html
  Reviewed-by: Eric Blake ebl...@redhat.com
v2:
  rebased
  added documentation changes
  mentioned testing in the commit message

 docs/virtio-balloon-stats.txt | 5 +++--
 hw/virtio/virtio-balloon.c| 7 ++-
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/docs/virtio-balloon-stats.txt b/docs/virtio-balloon-stats.txt
index f74612f..edff5f2 100644
--- a/docs/virtio-balloon-stats.txt
+++ b/docs/virtio-balloon-stats.txt
@@ -35,7 +35,8 @@ which will return a dictionary containing:
 
   o A key named last-update, which contains the last stats update
 timestamp in seconds. Since this timestamp is generated by the host,
-a buggy guest can't influence its value
+a buggy guest can't influence its value. The value is 0 if the guest
+has not updated the stats (yet).
 
 It's also important to note the following:
 
@@ -49,7 +50,7 @@ It's also important to note the following:
 
  - Polling can be enabled even if the guest doesn't have stats support
or the balloon driver wasn't loaded in the guest. If this is the case
-   and stats are queried, an error will be returned
+   and stats are queried, last-update will be 0.
 
  - The polling timer is only re-armed when the guest responds to the
statistics request. This means that if a (buggy) guest doesn't ever
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index bf2b588..22cd52e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -112,11 +112,6 @@ static void balloon_stats_get_all(Object *obj, struct 
Visitor *v,
 VirtIOBalloon *s = opaque;
 int i;
 
-if (!s-stats_last_update) {
-error_setg(errp, guest hasn't updated any stats yet);
-return;
-}
-
 visit_start_struct(v, NULL, guest-stats, name, 0, err);
 if (err) {
 goto out;
@@ -378,6 +373,8 @@ static void virtio_balloon_device_realize(DeviceState *dev, 
Error **errp)
 s-dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
 s-svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
 
+reset_stats(s);
+
 register_savevm(dev, virtio-balloon, -1, 1,
 virtio_balloon_save, virtio_balloon_load, s);
 
-- 
1.8.3.2

[Qemu-devel] [PATCH] virtio-balloon: return empty data when no stats are available

[Ján Tomko]

If the guest hasn't updated the stats yet, instead of returning
an error, return '-1' for the stats and '0' as 'last-update'.

This lets applications ignore this without parsing the error message.

Related libvirt patch and discussion:
https://www.redhat.com/archives/libvir-list/2014-May/msg00460.html

Signed-off-by: Ján Tomko jto...@redhat.com
---
 hw/virtio/virtio-balloon.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 971a921..6661c53 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -111,11 +111,6 @@ static void balloon_stats_get_all(Object *obj, struct 
Visitor *v,
 VirtIOBalloon *s = opaque;
 int i;
 
-if (!s-stats_last_update) {
-error_setg(errp, guest hasn't updated any stats yet);
-return;
-}
-
 visit_start_struct(v, NULL, guest-stats, name, 0, errp);
 visit_type_int(v, s-stats_last_update, last-update, errp);
 
@@ -361,6 +356,8 @@ static void virtio_balloon_device_realize(DeviceState *dev, 
Error **errp)
 s-dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
 s-svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
 
+reset_stats(s);
+
 register_savevm(dev, virtio-balloon, -1, 1,
 virtio_balloon_save, virtio_balloon_load, s);
 
-- 
1.8.3.2

Re: [Qemu-devel] [libvirt] [PATCHv2] Don't log an internal error when the guest hasn't updated balloon stats

[Ján Tomko]

On 05/16/2014 03:13 PM, Luiz Capitulino wrote:
 On Fri, 16 May 2014 00:11:24 -0600
 Eric Blake ebl...@redhat.com wrote:
 
 Is no stats yet really an error? 
 
 This is a special case where the guest hasn't ever filled QEMU with balloon
 stats. There are two possible cases. Either the guest hasn't done it yet, but
 will do in the future or the guest will never do it (eg. the guest doesn't
 support balloon, the guest crashed, etc).
 
 Libvirt has done nothing wrong, and
 I'd argue the guest hasn't done anything wrong, either.  Should we
 simply return an empty result?  Like cat on a file that hasn't gotten
 its data, yet.

 Yes, that would be reasonable.
 
 I'm fine with the two possible solutions here: adding a new TryAgain error
 class or returning an empty result.
 
 I say empty because those fields are not optionals, so we'll have to fill
 them with some value. Shouldn't be a problem for most fields, as the spec
 (docs/virtio-balloon-stats.txt) already defines that stats that the guest
 doesn't report are returned as -1. The only exception here is the last-update
 field, which can't hold a negative iirc. The only choice is to return 0 there.
 I guess that this shouldn't be a problem either.
 
 Who volunteers to fix this?
 

I've tried:

http://marc.info/?l=qemu-develm=140048179520115w=2

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [libvirt] qemu leaving unix sockets behind after VM is shut down

[Ján Tomko]

On 05/06/2014 03:39 PM, Stefan Hajnoczi wrote:
 On Tue, Apr 01, 2014 at 02:34:58PM -0600, Chris Friesen wrote:
 When running qemu with something like this

 -device virtio-serial \
 -chardev socket,path=/tmp/foo,server,nowait,id=foo \
 -device virtserialport,chardev=foo,name=host.port.0

 the VM starts up as expected and creates a socket at /tmp/foo as expected.

 However, when I shut down the VM the socket at /tmp/foo is left
 behind in the filesystem.  Basically qemu has leaked a file.

 With something like OpenStack where we could be creating/destroying
 many VMs this could end up creating a significant number of files in
 the specified directory.

 Has any thought been given to either automatically cleaning up the
 unix socket in the filesystem when qemu exits, or else supporting
 the abstract namespace for unix sockets to allow for automatic
 cleanup?

I have sent a libvirt patch to clean up the sockets on qemu shutdown:
https://www.redhat.com/archives/libvir-list/2014-May/msg00398.html

Jan

 
 Libvirt has a special case for the monitor socket in its
 qemuProcessStop() function.
 
 Are you using the OpenStack libvirt driver?
 
 Perhaps QEMU should support cleanup but first I think we should check
 the situation with libvirt.
 
 Stefan
 
 --
 libvir-list mailing list
 libvir-l...@redhat.com
 https://www.redhat.com/mailman/listinfo/libvir-list
 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [libvirt] Configure virtio-scsi options via libvirt

[Ján Tomko]

On 05/07/2014 01:13 AM, Mike Perez wrote:
 Hi everyone,
 
 I would like be able to configure virtio-scsi options num_queues, max_sectors,
 and cmd_per_lun via libvirt. Are there any plans to have this support?
 

num_queues is supported for virtio-scsi controllers since libvirt 1.0.5:

controller type='scsi' index='0' model='virtio-scsi'
  driver queues='8'/
/controller

http://libvirt.org/git/?p=libvirt.git;a=commit;h=d4bf0a9

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] Should we have a 2.0-rc3 ?

[Ján Tomko]

On 04/10/2014 02:46 PM, Alexander Graf wrote:
 
 On 10.04.2014, at 14:44, Eric Blake ebl...@redhat.com wrote:
 
 On 04/10/2014 05:17 AM, Peter Maydell wrote:
 So far I know of at least three fixes which should probably
 go into 2.0:
 * my fix for the configure stack-protector checks on MacOSX
 * MST's pull request updating the ACPI test blobs
 * MST says we need to update the hex files for ACPI too
   (otherwise you get a different ACPI blob depending on whether
your build system had iasl or not, if I understand correctly)

 Are there any others?

 Yes.  The libvirt team is a bit annoyed that the pci bus naming was
 changed for PPC but not all architectures, but without a proper QMP
 command to probe which naming scheme is in effect.  We thought that the
 naming scheme was going to be universally supplied for all arches, not
 just PPC.

 https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg01533.html

 Is this something that can be quickly fixed (perhaps by reverting the
 PPC patch until a more complete solution is ready), and if so, is it
 worth doing for 2.0 proper, rather than waiting for 2.0.1?
 
 Which way works better for you? I'd be perfectly fine with reverting the 
 patch. Libvirt is the only reason that path is there in the first place.
 

If I read the git history correctly, there were two patches changing pci bus
names for ppc in this release, not just one:

commit 1b8601b0ea0b91467561e0bbddd52a833e4b2b1a
Author: Alexey Kardashevskiy a...@ozlabs.ru
AuthorDate: 2014-03-06 14:11:00 +1100
Commit: Andreas Färber afaer...@suse.de
CommitDate: 2014-03-12 20:13:02 +0100

spapr-pci: Change the default PCI bus naming

Previously libvirt required the first/default PCI bus to have name pci.
Since QEMU can support multiple buses now, libvirt wants pci.0 now.

This removes custom bus name and lets QEMU make up default names.

Signed-off-by: Alexey Kardashevskiy a...@ozlabs.ru
Signed-off-by: Andreas Färber afaer...@suse.de

commit 8a0e11045d5f50d300e0ab1ba05f4c8217fb5dcb
Author: Alexander Graf ag...@suse.de
AuthorDate: 2013-12-04 12:42:32 +0100
Commit: Alexander Graf ag...@suse.de
CommitDate: 2013-12-20 01:58:01 +0100

PPC: Use default pci bus name for grackle and heathrow

There's no good reason to call our bus pci rather than let the default
bus name take over (pci.0).

The big downside to calling it different from anyone else is that tools
that pass -device get confused. They are looking for a bus pci.0 rather
than pci.

To make life easier for everyone, let's just drop the name override.

Signed-off-by: Alexander Graf ag...@suse.de

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] qcow2: Change default for new images to compat=1.1

[Ján Tomko]

On 08/19/2013 11:25 AM, Kevin Wolf wrote:
 By the time that qemu 1.7 will be released, enough time will has passed
 since qemu 1.1, which is the first version to understand version 3
 images, that changing the default shouldn't hurt many people any more
 and the benefits of using the new format outweigh the pain.
 
 qemu-iotests already runs with compat=1.1 by default.
 
 Signed-off-by: Kevin Wolf kw...@redhat.com
 ---
  block/qcow2.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/block/qcow2.c b/block/qcow2.c
 index 3376901..42ea7ec 100644
 --- a/block/qcow2.c
 +++ b/block/qcow2.c
 @@ -1402,7 +1402,7 @@ static int qcow2_create(const char *filename, 
 QEMUOptionParameter *options)
  int flags = 0;
  size_t cluster_size = DEFAULT_CLUSTER_SIZE;
  int prealloc = 0;
 -int version = 2;
 +int version = 3;
  
  /* Read out options */
  while (options  options-name) {


This does not affect qemu-img (or bdrv_img_create), as it gets overwritten
with 2 when BLOCK_OPT_COMPAT_LEVEL is present in options, but the value is NULL.

Should this go into qcow2_create_options[] as well?

Jan

Re: [Qemu-devel] [PATCH 1/2] qemu-socket: allow hostnames starting with a digit

[Ján Tomko]

On 06/18/2013 11:42 AM, Paolo Bonzini wrote:
 Il 03/06/2013 17:54, Ján Tomko ha scritto:
 According to RFC 1123 [1], hostnames can start with a digit too.

 [1] http://tools.ietf.org/html/rfc1123#page-13

 Signed-off-by: Ján Tomko jto...@redhat.com
 ---

  } else {
 -/* hostname */
 +/* hostname or IPv4 addr */
  if (2 != sscanf(str, %64[^:]:%32[^,]%n, host, port, pos)) {
  error_setg(errp, error parsing address '%s', str);
  goto fail;
  }
 +if (strcspn(host, 0123456789.) == 0) {
 
 I think what you want here is:
 
 if (host[strspn(host, 0123456789.)] == '\0') {
 

Yes, thank you for catching that.

Jan

 Otherwise, you're still basically testing
 
   qemu_isdigit(str[0]) || str[0] == '.'
 
 Paolo
 
 +addr-ipv4 = addr-has_ipv4 = true;
 +}
  }
  
  addr-host = g_strdup(host);

 
 

Re: [Qemu-devel] [PATCH 2/2] nbd: strip braces from literal IPv6 address in URI

[Ján Tomko]

On 06/04/2013 11:27 PM, Paolo Bonzini wrote:
 Il 03/06/2013 17:54, Ján Tomko ha scritto:
 Otherwise they would get passed to getaddrinfo and fail with:
 address resolution failed for [::1]:1234: Name or service not known
 
 Hmm... Hai Huang found a similar problem:
 
 error: internal error unable to execute QEMU command 'nbd-server-start':
 address resolution failed for [::]:5900: Name or service not known
 

That one should be fixed in libvirt-1.0.5.2 and libvirt-1.0.6 now. [1]

 This one is a libvirt bug, but perhaps it's simpler to just have a
 wrapper for getaddrinfo that strips brackets (and not strip the brackets
 in inet_parse, too).

I'm not sure if there are places other than calling getaddrinfo where having
brackets would be bad, I'm not that familiar with QEMU code.

I've thought about stripping the brackets in uri_parse too, as we do in
libvirt when parsing URIs. This wouldn't fix the case of someone specifying a
bracket-ecaped hostname where there is no need for escaping it, like the
libvirt bug.

And my patch is incomplete, because there are other block drivers calling
uri_parse without stripping the brackets.

Jan

[1] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=3accd7eb

Re: [Qemu-devel] [libvirt] NBD drives with literal IPv6 addresses or hostnames starting with a digit

[Ján Tomko]

On 06/04/2013 02:19 PM, Stefan Hajnoczi wrote:
 
 CCing Kevin who authored v1.4.0-736-gf17c90b.
 
 Stefan
 

I've already posted patches for both issues:
http://lists.nongnu.org/archive/html/qemu-devel/2013-06/msg00227.html

Jan

[Qemu-devel] [PATCH 0/2] Fix NBD hostname parsing issues

[Ján Tomko]

Fix parsing of NBD URIs with IPv6 addresses, broken by v1.4.0-736-gf17c90b
and fix parsing of NBD filenames with hostnames starting with a digit (fixed
by the same commit, but only for NBD URIs):
https://lists.gnu.org/archive/html/qemu-devel/2013-05/msg04719.html

Ján Tomko (2):
  qemu-socket: allow hostnames starting with a digit
  nbd: strip braces from literal IPv6 address in URI

 block/nbd.c | 11 ++-
 util/qemu-sockets.c | 13 -
 2 files changed, 14 insertions(+), 10 deletions(-)

-- 
1.8.1.5

[Qemu-devel] [PATCH 1/2] qemu-socket: allow hostnames starting with a digit

[Ján Tomko]

According to RFC 1123 [1], hostnames can start with a digit too.

[1] http://tools.ietf.org/html/rfc1123#page-13

Signed-off-by: Ján Tomko jto...@redhat.com
---
 util/qemu-sockets.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index fdd8dc4..727dafa 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -24,7 +24,6 @@
 
 #include monitor/monitor.h
 #include qemu/sockets.h
-#include qemu-common.h /* for qemu_isdigit */
 #include qemu/main-loop.h
 
 #ifndef AI_ADDRCONFIG
@@ -511,19 +510,15 @@ InetSocketAddress *inet_parse(const char *str, Error 
**errp)
 goto fail;
 }
 addr-ipv6 = addr-has_ipv6 = true;
-} else if (qemu_isdigit(str[0])) {
-/* IPv4 addr */
-if (2 != sscanf(str, %64[0-9.]:%32[^,]%n, host, port, pos)) {
-error_setg(errp, error parsing IPv4 address '%s', str);
-goto fail;
-}
-addr-ipv4 = addr-has_ipv4 = true;
 } else {
-/* hostname */
+/* hostname or IPv4 addr */
 if (2 != sscanf(str, %64[^:]:%32[^,]%n, host, port, pos)) {
 error_setg(errp, error parsing address '%s', str);
 goto fail;
 }
+if (strcspn(host, 0123456789.) == 0) {
+addr-ipv4 = addr-has_ipv4 = true;
+}
 }
 
 addr-host = g_strdup(host);
-- 
1.8.1.5

[Qemu-devel] [PATCH 2/2] nbd: strip braces from literal IPv6 address in URI

[Ján Tomko]

Otherwise they would get passed to getaddrinfo and fail with:
address resolution failed for [::1]:1234: Name or service not known

(Broken by commit v1.4.0-736-gf17c90b)

Signed-off-by: Ján Tomko jto...@redhat.com
---
 block/nbd.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/nbd.c b/block/nbd.c
index 30e3b78..9c480b8 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -118,13 +118,22 @@ static int nbd_parse_uri(const char *filename, QDict 
*options)
 }
 qdict_put(options, path, qstring_from_str(qp-p[0].value));
 } else {
+QString *host;
 /* nbd[+tcp]://host[:port]/export */
 if (!uri-server) {
 ret = -EINVAL;
 goto out;
 }
 
-qdict_put(options, host, qstring_from_str(uri-server));
+/* strip braces from literal IPv6 address */
+if (uri-server[0] == '[') {
+host = qstring_from_substr(uri-server, 1,
+   strlen(uri-server) - 2);
+} else {
+host = qstring_from_str(uri-server);
+}
+
+qdict_put(options, host, host);
 if (uri-port) {
 char* port_str = g_strdup_printf(%d, uri-port);
 qdict_put(options, port, qstring_from_str(port_str));
-- 
1.8.1.5

[Qemu-devel] NBD drives with literal IPv6 addresses or hostnames starting with a digit

[Ján Tomko]

Hello,

since qemu's commit v1.4.0-736-gf17c90b [1]:
nbd: Keep hostname and port separate

* literal IPv6 addresses no longer work in nbd URIs, because getaddrinfo is
called with the surrounding brackets:
$ qemu-system-x86_64 -drive file=nbd://[::1]:1234/quack
qemu-system-x86_64: -drive file=nbd://[::1]:1234/quack: address resolution
failed for [::1]:1234: Name or service not known

* hostnames starting with a digit now work in nbd URIs.
Before that, or with the non-URI syntax, they fail because inet_parse assumes
them to be literal IPv4 addresses:
$ qemu-system-x86_64 -drive file=nbd:123flour:1234:exportname=gashunk
qemu-system-x86_64: -drive file=nbd:123flour:1234:exportname=gashunk: error
parsing IPv4 address '123flour:1234'


In libvirt, we use the URI format on the command line only if the host
contains ':', so hostnames starting with a digit still don't work.

Migration with NBD and a literal IPv6 address doesn't work either, but that's
purely libvirt's fault, as we don't escape it with brackets. I've just posted
a patch for that. [2]

Jan

[1] http://git.qemu.org/?p=qemu.git;a=commitdiff;h=f17c90b
[2] https://www.redhat.com/archives/libvir-list/2013-May/msg02022.html

Re: [Qemu-devel] [RFC] qcow3 format in libvirt

[Ján Tomko]

On 03/04/2013 04:40 PM, Kevin Wolf wrote:
 Am 04.03.2013 um 16:19 hat Daniel P. Berrange geschrieben:
 On Mon, Mar 04, 2013 at 04:05:50PM +0100, Kevin Wolf wrote:

 I'm not talking about the QEMU cli, but about qcow2 as the format as
 defined in the spec (which just happens to sit in qemu.git, but isn't
 qemu specific at all)

 So you're saying that you consider the format name to be qcow2 regardless
 of whether the version numer field is specified as 2 or 3.
 
 Yes.

 So in other words, if an application came along and required libvirt to
 set  format=qcow3 on its CLI, we could justifiably refuse to do that in
 libvirt claiming this is not in compliance with the spec ?
 
 No, you would just check which features this image uses (which, if I
 understood correctly, you need to save anyway), and if a version 3
 feature is among it (the basic version 3 could be represented by either
 a feature flags or zero clusters feature, which are what version 3
 really means), pass it the 'qcow3' command line option it wants.
 

Since libvirt needs to know the feature names, I doesn't seem to be a
problem to know what compat options they need. And the empty (but
present) features element could just mean compat=1.1. Would we still
need to support creating compat=0.10 images with older qemu-img not
understanding this option after the default gets changed in the current
version?

 Of course, I would be disappointed that the tool thought it had to
 invent format names, but it's not really blocking any functionality.
 
 Just the same way it could happen that a tool uses different drivers for
 other features that we introduce. For example, imagine that we introduce
 a flag that modifies the L2 table structure to allow subclusters - a
 change that we've been discussing before and that would have a massive
 impact on the implementation, even though it's only a feature flag that
 has changed, and not the version number. Using a different driver for
 this looks much more likely than a different driver for version 2 and 3,
 which was really a quite small step.

So, the format and the driver is still 'qcow2' now, there's no need to
translate anything at the moment (apart from features-options when
creating the image).

But if the same 'qcow2' format would need different drivers based on the
features, we need to use different set of values for driver types and
image formats, if the features would not be in the domain XML.

 
 So the main problem that I see is the assumption different version =
 big change, new feature flag = small change and as a conclusion from
 that different version = possibly new driver, new feature flag =
 definitely only old driver. This isn't true at all.
 
 Kevin
 

[Qemu-devel] [RFC] qcow3 format in libvirt

[Ján Tomko]

Before posting another version of my patches [1], attempting to add
support for the new qcow format to libvirt, I would like to know if this
sounds reasonable:

A new format named 'qcow3' would be added, along with a features
sub-element for target.

volume
  nameqcow3test/name
  source
  /source
  capacity unit='GiB'8/capacity
  target
path/var/lib/libvirt/images/qcow3test/path
format type='qcow3'/
features
  lazy_refcounts/
/features
  /target
/volume

I think that libvirt shouldn't care if the features are compatible or
incompatible, as we don't know what features are supported by the
hypervisor. Would the features be any good as tri-state (on, off, default?).

While the qcow3 format is handled by the qcow2 driver in QEMU,
driver name='qemu' type='qcow2'/ should be enough for domains,
but in snapshot XML we treat the driver type as the format:

disk name='/path/to/old'
  driver type='qcow3'/
  source file='/path/to/new'/
  features
lazy_refcounts/
  /features
/disk

So I think we should allow the qcow3 driver type as well and translate
it to qcow2 for QEMU.

Jan

[1] v2 here:
https://www.redhat.com/archives/libvir-list/2013-February/msg00212.html