Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
DanPB, Could you please reiterate the suggestion regarding the placement of the pci root hotplug xml you made in the irc channel? On Fri, Sep 24, 2021 at 02:16 Laine Stump wrote: > On 9/11/21 11:26 PM, Ani Sinha wrote: > > Hi all: > > > > This patchset introduces libvirt xml support for the following two pm > conf > > options: > > > > > > > > > > > > (before I get into a more radical discussion about different options - > since we aren't exactly duplicating the QEMU option name anyway, what if > we made these names more consistent, e.g. "acpi-hotplug-bridge" and > "acpi-hotplug-root"?) > > I've thought quite a bit about whether to put these attributes here, or > somewhere else, and I'm still undecided. > > My initial reaction to this was "PM == Power Management, and power > management is all about suspend mode support. Hotplug isn't power > management." But then you look at the name of the QEMU option and PM is > right there in the name, and I guess it's *kind of related* (effectively > suspending/resuming a single device), so maybe I'm thinking too narrowly. > > So are there alternate places that might fit the purpose of these new > options better, rather than directly mimicking the QEMU option placement > (for better or worse)? A couple alternative possibilities: > > 1) > > One possibility would be to include these new flags within the existing > subelement of , which is already used to control > whether the guest exposes ACPI to the guest *at all* (via adding > "-no-acpi" to the QEMU commandline when is missing - NB: this > feature flag is currently supported only on x86 and aarch64 QEMU > platforms, and ignored for all other hypervisors). > > Possibly the new flags could be put in something like this: > > > > > > >... > > > But: > > * currently there are no subelements to . So this isn't "extending > according to an existing pattern". > > * even though the element uses presence of a subelement to > indicate "enabled" and absence of the subelement to indicate "disabled". > But in the case of these new acpi bridge options we would need to > explicitly have the "enabled='yes/no'" rather than just using presence > of the option to mean "enabled" and absence to mean "disabled" because > the default for "root-hotplug" up until now has been *enabled*, and the > default for hotplug-bridge is different depending on machinetype. We > need to continue working properly (and identically) with old/existing > XML, but if we didn't have an "enabled" attribute for these new flags, > there would be no way to tell the difference between "not specified" and > "disabled", and so no way to disable the feature for a QEMU where the > default was "enabled". (Why does this matter? Because I don't like the > inconsistency that would arise from some feature flags using absense to > mean "disabled" and some using it to mean "use the default".) > > * Having something in in the domain XML kind of implies that > the associated capability flags should be represented in the > section of the domain capabilities. For example, is listed under > in the output of virsh capabilities, separately from the flag > indicating presence of the -no-acpi option. I'm not sure if we would > need to add something there for these options if we moved them into > (seems a bit redundant to me to have it in both places, but > I'm sure there are $reasons). > > > 2) * > > Alternately, there is an subelement of , which is currently > used to add a SLIC table (some sort of software license table, which I'd > never heard of before) using QEMU's -acpitable commandline option. It is > also used somehow by the Xen driver. > > > > /path/to/slic.dat > > > >... > > > My problem with adding these new PCI controller acpi options to os/acpi > is simply that it's in the subelement, which is claimed elsewhere > to be intended for OS boot options, and is used for things like > specifying the path to a kernel / initrd to boot from. > > 3) > > A third option, suggested somewhere by Ani, would be to make a > completely new top-level element, called something like > that would have separate attributes for the two flags, e.g.: > > > > I dislike new toplevel options because they just seem so adhoc, as if > the XML namespace is a cluttered, disorganized room. That reminds me too > much of my own workspace, which is just... depressing. > > > > Since I always seem to spend *way too much time* worrying about naming, > only to have it come out wrong in the end anyway, I'm looking for some > other opinions. Counting the version that is in Ani's patch currently as > option "0", which option do you all think is the best? Or is it > completely unimportant? > > > The above two options are only available for qemu driver and that too > for x86 > > guests only. Both of them are global options. > > > > ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support > for cold > > plu
Re: [PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts
24.09.2021 18:10, Kevin Wolf wrote:
Am 24.09.2021 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
24.09.2021 12:04, Kevin Wolf wrote:
DeviceState.id is a pointer to a string that is stored in the QemuOpts
object DeviceState.opts and freed together with it. We want to create
devices without going through QemuOpts in the future, so make this a
separately allocated string.
Signed-off-by: Kevin Wolf
Interesting that in hw/xen/xen-legacy-backend.c
g_strdup_printf-allocated id is passed to qdev_set_id prior this
patch. So, the patch seems to fix that small leak. Worth to mention?
Ok, I can mention it explicitly.
---
include/hw/qdev-core.h | 2 +-
include/monitor/qdev.h | 2 +-
hw/arm/virt.c | 2 +-
hw/core/qdev.c | 1 +
hw/pci-bridge/pci_expander_bridge.c | 2 +-
hw/ppc/e500.c | 2 +-
softmmu/qdev-monitor.c | 5 +++--
7 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 34c8a7506a..1857d9698e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -176,7 +176,7 @@ struct DeviceState {
Object parent_obj;
/*< public >*/
-const char *id;
+char *id;
char *canonical_path;
bool realized;
bool pending_deleted_event;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index eaa947d73a..389287eb44 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, const char *id);
+void qdev_set_id(DeviceState *dev, char *id);
#endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1d59f0e59f..f933d48d3b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
MemoryRegion *sysmem = get_system_memory();
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
qdev_prop_set_uint32(dev, "mmio_size",
vms->memmap[VIRT_PLATFORM_BUS].size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a..d918b50a1d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
}
qemu_opts_del(dev->opts);
+g_free(dev->id);
}
static void device_class_base_init(ObjectClass *class, void *data)
diff --git a/hw/pci-bridge/pci_expander_bridge.c
b/hw/pci-bridge/pci_expander_bridge.c
index 7112dc3062..10e6e7c2ab 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool
pcie, Error **errp)
} else {
bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
TYPE_PXB_BUS);
bds = qdev_new("pci-bridge");
-bds->id = dev_name;
+bds->id = g_strdup(dev_name);
qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR,
pxb->bus_nr);
qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
}
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 95451414dd..960e7efcd3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
/* Platform Bus Device */
if (pmc->has_platform_bus) {
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 034b999401..1207e57a46 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
return bus;
}
-void qdev_set_id(DeviceState *dev, const char *id)
+/* Takes ownership of @id, will be freed when deleting the device */
+void qdev_set_id(DeviceState *dev, char *id)
{
if (id) {
dev->id = id;
@@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, qemu_opts_id(opts));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
In hw/pci/pci_bridge.c
we do
br->bus_name = dev->qdev.id
It seems bad, as we now stole dynamic pointer, that may be freed later.
If it's bad now, it was as
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
On Fri, Sep 24, 2021 at 11:04:27AM +0200, Kevin Wolf wrote: > We want to switch both from QemuOpts to the keyval parser in the future, > which results in some incompatibilities, mainly around list handling. > Mark the non-JSON version of both as unstable syntax so that management > tools switch to JSON and we can later make the change without breaking > things. > > Signed-off-by: Kevin Wolf > --- > docs/about/deprecated.rst | 11 +++ > 1 file changed, 11 insertions(+) Reviewed-by: Eric Blake > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 3c2be84d80..42f6a478fb 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -160,6 +160,17 @@ Use ``-display sdl`` instead. > > Use ``-display curses`` instead. > > +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) > +'' > + > +If you rely on a stable interface for ``-device`` and ``-object`` that > doesn't > +change incompatibly between QEMU versions (e.g. because you are using the > QEMU > +command line as a machine interface in scripts rather than interactively), > use > +JSON syntax for these options instead. > + > +There is no intention to remove support for non-JSON syntax entirely, but > +future versions may change the way to spell some options. > + > > Plugin argument passing through ``arg=`` (since 6.1) > > -- > 2.31.1 > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 10/11] vl: Enable JSON syntax for -device
On Fri, Sep 24, 2021 at 11:04:26AM +0200, Kevin Wolf wrote:
> Like we already do for -object, introduce support for JSON syntax in
> -device, which can be kept stable in the long term and guarantees that a
> single code path with identical behaviour is used for both QMP and the
> command line. Compared to the QemuOpts based code, the parser contains
> less surprises and has support for non-scalar options (lists and
> structs). Switching management tools to JSON means that we can more
> easily change the "human" CLI syntax from QemuOpts to the keyval parser
> later.
>
> In the QAPI schema, a feature flag is added to the device-add command to
> allow management tools to detect support for this.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/qdev.json | 15 +
> softmmu/vl.c | 58 --
> 2 files changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index b83178220b..cdc8f911b5 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -32,17 +32,23 @@
> ##
> # @device_add:
> #
> +# Add a device.
> +#
> # @driver: the name of the new device's driver
> #
> # @bus: the device's parent bus (device tree path)
> #
> # @id: the device's ID, must be unique
> #
> -# Additional arguments depend on the type.
> -#
> -# Add a device.
> +# Features:
> +# @json-cli: If present, the "-device" command line option supports JSON
> +#syntax with a structure identical to the arguments of this
> +#command.
> #
> # Notes:
> +#
> +# Additional arguments depend on the type.
> +#
> # 1. For detailed information about this command, please refer to the
> #'docs/qdev-device-use.txt' file.
> #
> @@ -67,7 +73,8 @@
> ##
> { 'command': 'device_add',
>'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
> - 'gen': false } # so we can get the additional arguments
> + 'gen': false, # so we can get the additional arguments
> + 'features': ['json-cli'] }
Eventually, we'll get rid of this 'gen':false, but this patch series
is already an improvement towards that goal.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
On Fri, Sep 24, 2021 at 11:04:25AM +0200, Kevin Wolf wrote:
> Directly call qdev_device_add_from_qdict() for QMP device_add instead of
> first going through QemuOpts and converting back to QDict.
>
> Note that this changes the behaviour of device_add, though in ways that
> should be considered bug fixes:
>
> QemuOpts ignores differences between data types, so you could
> successfully pass a string "123" for an integer property, or a string
> "on" for a boolean property (and vice versa). After this change, the
> correct data type for the property must be used in the JSON input.
>
> qemu_opts_from_qdict() also silently ignores any options whose value is
> a QDict, QList or QNull.
>
> To illustrate, the following QMP command was accepted before and is now
> rejected for both reasons:
>
> { "execute": "device_add",
> "arguments": { "driver": "scsi-cd",
> "drive": { "completely": "invalid" },
> "physical_block_size": "4096" } }
>
> Signed-off-by: Kevin Wolf
> ---
> softmmu/qdev-monitor.c | 18 +++---
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 08/11] qdev: Base object creation on QDict rather than QemuOpts
On Fri, Sep 24, 2021 at 11:04:24AM +0200, Kevin Wolf wrote: > QDicts are both what QMP natively uses and what the keyval parser > produces. Going through QemuOpts isn't useful for either one, so switch > the main device creation function to QDicts. By sharing more code with > the -object/object-add code path, we can even reduce the code size a > bit. > > This commit doesn't remove the detour through QemuOpts from any code > path yet, but it allows the following commits to do so. > > Signed-off-by: Kevin Wolf > --- > include/hw/qdev-core.h | 8 ++--- > hw/core/qdev.c | 5 ++-- > hw/net/virtio-net.c| 4 +-- > hw/vfio/pci.c | 4 +-- > softmmu/qdev-monitor.c | 67 +++--- > 5 files changed, 41 insertions(+), 47 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 0/3] remove sysconfig files
Hi All! I'd like to revive discussion on this series. It LGTM with the exception of a few small nits in 2/3. AFAICT, all concerns were addressed. Are there any objections to removing these files? Have all distro maintainers spoken their peace? If there are no objections, it would be nice to get this pushed early in the dev cycle, after the release of 7.8.0. On 8/2/21 05:18, Olaf Hering wrote: rebased to 919f25d36ef0ea41c50bdb5afa0b83187ffb3c87 Needs rebased again :-). Regards, Jim Olaf Hering (3): libvirt.spec: relocate pre script of daemon-driver-qemu remove sysconfig files NEWS: mention removal of sysconfig NEWS.rst| 10 +++ docs/daemons.rst| 20 + docs/remote.html.in | 2 +- libvirt.spec.in | 100 src/ch/meson.build | 5 -- src/ch/virtchd.service.in | 1 + src/ch/virtchd.sysconf | 3 - src/interface/meson.build | 5 -- src/interface/virtinterfaced.service.in | 1 + src/interface/virtinterfaced.sysconf| 3 - src/libxl/meson.build | 5 -- src/libxl/virtxend.service.in | 1 + src/libxl/virtxend.sysconf | 3 - src/locking/meson.build | 5 -- src/locking/virtlockd.service.in| 1 + src/locking/virtlockd.sysconf | 3 - src/logging/meson.build | 5 -- src/logging/virtlogd.sysconf| 3 - src/lxc/meson.build | 5 -- src/lxc/virtlxcd.service.in | 1 + src/lxc/virtlxcd.sysconf| 3 - src/meson.build | 16 src/network/meson.build | 5 -- src/network/virtnetworkd.service.in | 1 + src/network/virtnetworkd.sysconf| 3 - src/node_device/meson.build | 5 -- src/node_device/virtnodedevd.service.in | 1 + src/node_device/virtnodedevd.sysconf| 3 - src/nwfilter/meson.build| 5 -- src/nwfilter/virtnwfilterd.service.in | 1 + src/nwfilter/virtnwfilterd.sysconf | 3 - src/qemu/meson.build| 5 -- src/qemu/virtqemud.service.in | 7 ++ src/qemu/virtqemud.sysconf | 12 --- src/remote/libvirtd.service.in | 7 ++ src/remote/libvirtd.sysconf | 21 - src/remote/meson.build | 10 --- src/remote/virtproxyd.service.in| 1 + src/remote/virtproxyd.sysconf | 3 - src/secret/meson.build | 5 -- src/secret/virtsecretd.service.in | 1 + src/secret/virtsecretd.sysconf | 3 - src/storage/meson.build | 5 -- src/storage/virtstoraged.service.in | 1 + src/storage/virtstoraged.sysconf| 3 - src/vbox/meson.build| 5 -- src/vbox/virtvboxd.service.in | 1 + src/vbox/virtvboxd.sysconf | 3 - src/vz/meson.build | 5 -- src/vz/virtvzd.service.in | 1 + src/vz/virtvzd.sysconf | 3 - tools/libvirt-guests.sh.in | 40 ++ tools/libvirt-guests.sysconf| 50 tools/meson.build | 6 -- 54 files changed, 166 insertions(+), 260 deletions(-) delete mode 100644 src/ch/virtchd.sysconf delete mode 100644 src/interface/virtinterfaced.sysconf delete mode 100644 src/libxl/virtxend.sysconf delete mode 100644 src/locking/virtlockd.sysconf delete mode 100644 src/logging/virtlogd.sysconf delete mode 100644 src/lxc/virtlxcd.sysconf delete mode 100644 src/network/virtnetworkd.sysconf delete mode 100644 src/node_device/virtnodedevd.sysconf delete mode 100644 src/nwfilter/virtnwfilterd.sysconf delete mode 100644 src/qemu/virtqemud.sysconf delete mode 100644 src/remote/libvirtd.sysconf delete mode 100644 src/remote/virtproxyd.sysconf delete mode 100644 src/secret/virtsecretd.sysconf delete mode 100644 src/storage/virtstoraged.sysconf delete mode 100644 src/vbox/virtvboxd.sysconf delete mode 100644 src/vz/virtvzd.sysconf delete mode 100644 tools/libvirt-guests.sysconf
Re: [PATCH v3 2/3] remove sysconfig files
On 8/2/21 05:18, Olaf Hering wrote: sysconfig files are owned by the admin of the host. They have the liberty to put anything they want into these files. This makes it difficult to provide different built-in defaults. Remove the sysconfig file and place the current desired default into the service file. Local customizations can now go either into /etc/sysconfig/name or /etc/systemd/system/name.service.d/my-knobs.conf Attempt to handle upgrades in libvirt.spec. Dirty files which are marked as %config will be renamed to file.rpmsave. To restore them automatically, move stale .rpmsave files away, and catch any new rpmsave files in %posttrans. Signed-off-by: Olaf Hering --- v2: - fix libvirt_sc_pre usage - leave existing grep in sysconfig in %posttrans daemon as it is - leave existing %post daemon as it is - add empty VIRTLOCKD_ARGS= to virtlockd.service - provide drop-in example in daemons.rst --- docs/daemons.rst| 20 docs/remote.html.in | 2 +- libvirt.spec.in | 68 +++-- src/ch/meson.build | 5 -- src/ch/virtchd.service.in | 1 + src/ch/virtchd.sysconf | 3 -- src/interface/meson.build | 5 -- src/interface/virtinterfaced.service.in | 1 + src/interface/virtinterfaced.sysconf| 3 -- src/libxl/meson.build | 5 -- src/libxl/virtxend.service.in | 1 + src/libxl/virtxend.sysconf | 3 -- src/locking/meson.build | 5 -- src/locking/virtlockd.service.in| 1 + src/locking/virtlockd.sysconf | 3 -- src/logging/meson.build | 5 -- src/logging/virtlogd.sysconf| 3 -- src/lxc/meson.build | 5 -- src/lxc/virtlxcd.service.in | 1 + src/lxc/virtlxcd.sysconf| 3 -- src/meson.build | 16 -- src/network/meson.build | 5 -- src/network/virtnetworkd.service.in | 1 + src/network/virtnetworkd.sysconf| 3 -- src/node_device/meson.build | 5 -- src/node_device/virtnodedevd.service.in | 1 + src/node_device/virtnodedevd.sysconf| 3 -- src/nwfilter/meson.build| 5 -- src/nwfilter/virtnwfilterd.service.in | 1 + src/nwfilter/virtnwfilterd.sysconf | 3 -- src/qemu/meson.build| 5 -- src/qemu/virtqemud.service.in | 7 +++ src/qemu/virtqemud.sysconf | 12 - src/remote/libvirtd.service.in | 7 +++ src/remote/libvirtd.sysconf | 21 src/remote/meson.build | 10 src/remote/virtproxyd.service.in| 1 + src/remote/virtproxyd.sysconf | 3 -- src/secret/meson.build | 5 -- src/secret/virtsecretd.service.in | 1 + src/secret/virtsecretd.sysconf | 3 -- src/storage/meson.build | 5 -- src/storage/virtstoraged.service.in | 1 + src/storage/virtstoraged.sysconf| 3 -- src/vbox/meson.build| 5 -- src/vbox/virtvboxd.service.in | 1 + src/vbox/virtvboxd.sysconf | 3 -- src/vz/meson.build | 5 -- src/vz/virtvzd.service.in | 1 + src/vz/virtvzd.sysconf | 3 -- tools/libvirt-guests.sh.in | 40 +++ tools/libvirt-guests.sysconf| 50 -- tools/meson.build | 6 --- 53 files changed, 141 insertions(+), 243 deletions(-) delete mode 100644 src/ch/virtchd.sysconf delete mode 100644 src/interface/virtinterfaced.sysconf delete mode 100644 src/libxl/virtxend.sysconf delete mode 100644 src/locking/virtlockd.sysconf delete mode 100644 src/logging/virtlogd.sysconf delete mode 100644 src/lxc/virtlxcd.sysconf delete mode 100644 src/network/virtnetworkd.sysconf delete mode 100644 src/node_device/virtnodedevd.sysconf delete mode 100644 src/nwfilter/virtnwfilterd.sysconf delete mode 100644 src/qemu/virtqemud.sysconf delete mode 100644 src/remote/libvirtd.sysconf delete mode 100644 src/remote/virtproxyd.sysconf delete mode 100644 src/secret/virtsecretd.sysconf delete mode 100644 src/storage/virtstoraged.sysconf delete mode 100644 src/vbox/virtvboxd.sysconf delete mode 100644 src/vz/virtvzd.sysconf delete mode 100644 tools/libvirt-guests.sysconf diff --git a/docs/daemons.rst b/docs/daemons.rst index 8a05ee77a7..1b6396d2af 100644 --- a/docs/daemons.rst +++ b/docs/daemons.rst @@ -686,3 +686,23 @@ socket unit names into the service. When using these old versions, the ``unix_sock_dir`` setting in ``virtlockd.conf`` must be changed in lock-step with the equivalent setting in the unit files to ensure that ``virtlockd`` can identify the sockets. + +Changing command line options for daemons
Re: [PATCH 04/11] qdev: Avoid using string visitor for properties
On Fri, Sep 24, 2021 at 11:04:20AM +0200, Kevin Wolf wrote: > The only thing the string visitor adds compared to a keyval visitor is > list support. git grep for 'visit_start_list' and 'visit.*List' shows > that devices don't make use of this. > > In a world with a QAPIfied command line interface, the keyval visitor is > used to parse the command line. In order to make sure that no devices > start using this feature that would make backwards compatibility harder, > just switch away from object_property_parse(), which internally uses the > string visitor, to a keyval visitor and object_property_set(). > > Signed-off-by: Kevin Wolf > --- > softmmu/qdev-monitor.c | 20 +--- > 1 file changed, 17 insertions(+), 3 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 07/11] qemu-option: Allow deleting opts during qemu_opts_foreach()
24.09.2021 12:04, Kevin Wolf wrote: Use QTAILQ_FOREACH_SAFE() so that the current QemuOpts can be deleted while iterating through the whole list. Signed-off-by: Kevin Wolf Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH 01/11] qom: Reduce use of error_propagate()
On Fri, Sep 24, 2021 at 11:04:17AM +0200, Kevin Wolf wrote: > ERRP_GUARD() makes debugging easier by making sure that &error_abort > still fails at the real origin of the error instead of > error_propagate(). > > Signed-off-by: Kevin Wolf > --- > qom/object.c| 7 +++ > qom/object_interfaces.c | 17 ++--- > 2 files changed, 9 insertions(+), 15 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()
24.09.2021 12:04, Kevin Wolf wrote:
object_property_add_child() fails (with &error_abort) if an object with
the same name already exists. As long as QemuOpts is in use for -device
and device_add, it catches duplicate IDs before qdev_set_id() is even
called. However, for enabling non-QemuOpts code paths, we need to make
sure that the condition doesn't cause a crash, but fails gracefully.
Signed-off-by: Kevin Wolf
---
include/monitor/qdev.h | 2 +-
hw/xen/xen-legacy-backend.c | 3 ++-
softmmu/qdev-monitor.c | 16 ++--
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..7961308c75 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+void qdev_set_id(DeviceState *dev, char *id, Error **errp);
#endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..17aca85ddc 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char
*type, int dom,
xendev = g_malloc0(ops->size);
object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
OBJECT(xendev)->free = g_free;
-qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
+qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+&error_abort);
qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal);
object_unref(OBJECT(xendev));
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..c2af906df0 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, Error **errp)
}
/* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+void qdev_set_id(DeviceState *dev, char *id, Error **errp)
According to recommendations in error.h, worth adding also return value (for
example true=success false=failure), so [..]
{
if (id) {
dev->id = id;
}
Unrelated but.. What's the strange logic?
Is it intended that with passed id=NULL we don't update dev->id variable but try
to do following logic with old dev->id?
if (dev->id) {
-object_property_add_child(qdev_get_peripheral(), dev->id,
- OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral(), dev->id,
+ OBJECT(dev), errp);
} else {
static int anon_count;
gchar *name = g_strdup_printf("device[%d]", anon_count++);
-object_property_add_child(qdev_get_peripheral_anon(), name,
- OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral_anon(), name,
+ OBJECT(dev), errp);
g_free(name);
}
}
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
{
+ERRP_GUARD();
DeviceClass *dc;
const char *driver, *path;
DeviceState *dev = NULL;
@@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
+if (*errp) {
+goto err_del_dev;
+}
[..] here we'll have
if (!qdev_set_id(...)) {
goto err_del_dev;
}
and no need for ERRP_GUARD.
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
--
Best regards,
Vladimir
Re: [PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts
24.09.2021 12:04, Kevin Wolf wrote:
DeviceState.id is a pointer to a string that is stored in the QemuOpts
object DeviceState.opts and freed together with it. We want to create
devices without going through QemuOpts in the future, so make this a
separately allocated string.
Signed-off-by: Kevin Wolf
Interesting that in hw/xen/xen-legacy-backend.c g_strdup_printf-allocated id is
passed to qdev_set_id prior this patch. So, the patch seems to fix that small
leak. Worth to mention?
---
include/hw/qdev-core.h | 2 +-
include/monitor/qdev.h | 2 +-
hw/arm/virt.c | 2 +-
hw/core/qdev.c | 1 +
hw/pci-bridge/pci_expander_bridge.c | 2 +-
hw/ppc/e500.c | 2 +-
softmmu/qdev-monitor.c | 5 +++--
7 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 34c8a7506a..1857d9698e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -176,7 +176,7 @@ struct DeviceState {
Object parent_obj;
/*< public >*/
-const char *id;
+char *id;
char *canonical_path;
bool realized;
bool pending_deleted_event;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index eaa947d73a..389287eb44 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, const char *id);
+void qdev_set_id(DeviceState *dev, char *id);
#endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1d59f0e59f..f933d48d3b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
MemoryRegion *sysmem = get_system_memory();
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
qdev_prop_set_uint32(dev, "mmio_size",
vms->memmap[VIRT_PLATFORM_BUS].size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a..d918b50a1d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
}
qemu_opts_del(dev->opts);
+g_free(dev->id);
}
static void device_class_base_init(ObjectClass *class, void *data)
diff --git a/hw/pci-bridge/pci_expander_bridge.c
b/hw/pci-bridge/pci_expander_bridge.c
index 7112dc3062..10e6e7c2ab 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool
pcie, Error **errp)
} else {
bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
TYPE_PXB_BUS);
bds = qdev_new("pci-bridge");
-bds->id = dev_name;
+bds->id = g_strdup(dev_name);
qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
}
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 95451414dd..960e7efcd3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
/* Platform Bus Device */
if (pmc->has_platform_bus) {
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 034b999401..1207e57a46 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
return bus;
}
-void qdev_set_id(DeviceState *dev, const char *id)
+/* Takes ownership of @id, will be freed when deleting the device */
+void qdev_set_id(DeviceState *dev, char *id)
{
if (id) {
dev->id = id;
@@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, qemu_opts_id(opts));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
In hw/pci/pci_bridge.c
we do
br->bus_name = dev->qdev.id
It seems bad, as we now stole dynamic pointer, that may be freed later.
--
Best regards,
Vladimir
Re: [PATCH 03/11] iotests/051: Fix typo
24.09.2021 12:04, Kevin Wolf wrote: The iothread isn't called 'iothread0', but 'thread0'. Depending on the order that properties are parsed, the error message may change from the expected one to another one saying that the iothread doesn't exist. Signed-off-by: Kevin Wolf Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH 02/11] iotests/245: Fix type for iothread property
24.09.2021 12:04, Kevin Wolf wrote: iothread is a string property, so None (= JSON null) is not a valid value for it. Pass the empty string instead to get the default iothread. Signed-off-by: Kevin Wolf Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH 01/11] qom: Reduce use of error_propagate()
24.09.2021 12:04, Kevin Wolf wrote:
ERRP_GUARD() makes debugging easier by making sure that &error_abort
still fails at the real origin of the error instead of
error_propagate().
Signed-off-by: Kevin Wolf
---
qom/object.c| 7 +++
qom/object_interfaces.c | 17 ++---
2 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index e86cb05b84..6be710bc40 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1389,7 +1389,7 @@ bool object_property_get(Object *obj, const char *name,
Visitor *v,
bool object_property_set(Object *obj, const char *name, Visitor *v,
Error **errp)
{
-Error *err = NULL;
+ERRP_GUARD();
ObjectProperty *prop = object_property_find_err(obj, name, errp);
if (prop == NULL) {
@@ -1400,9 +1400,8 @@ bool object_property_set(Object *obj, const char *name,
Visitor *v,
error_setg(errp, QERR_PERMISSION_DENIED);
return false;
}
-prop->set(obj, v, name, prop->opaque, &err);
-error_propagate(errp, err);
-return !err;
+prop->set(obj, v, name, prop->opaque, errp);
+return !*errp;
}
This is OK: prop->set doesn't return value, so we have to analyze errp to make
our own return value.
bool object_property_set_str(Object *obj, const char *name,
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ad9b56b59a..80691e88cd 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -45,26 +45,21 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
Visitor *v, Error **errp)
{
+ERRP_GUARD();
const QDictEntry *e;
-Error *local_err = NULL;
-if (!visit_start_struct(v, NULL, NULL, 0, &local_err)) {
-goto out;
+if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
+return;
}
for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-if (!object_property_set(obj, e->key, v, &local_err)) {
+if (!object_property_set(obj, e->key, v, errp)) {
break;
}
}
-if (!local_err) {
-visit_check_struct(v, &local_err);
+if (!*errp) {
+visit_check_struct(v, errp);
}
visit_end_struct(v, NULL);
-
-out:
-if (local_err) {
-error_propagate(errp, local_err);
-}
}
void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
ERRP_GUARD() + dereferencing errp is good when we use functions which don't
return any value. So we want to check is it fail or not and we have to analyze
errp.
But that is not the case: all called functions follow modern recommendations of
returning some value indicating failure together with setting errp.
I think, it's better not use ERRP_GUARD where it is not needed: in ideal world,
all functions that may fail do return value, and we don't need neither error
propagation, nor dereferencing errp. And don't need ERRP_GUARD. ERRP_GUARD
will be still needed to support error_prepend()/error_append() together with
error_fatal, but again that's not the case.
Here I suggest something like this:
static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
Visitor *v, Error **errp)
{
const QDictEntry *e;
if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
return;
}
for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
if (!object_property_set(obj, e->key, v, errp)) {
goto out;
}
}
visit_check_struct(v, errp);
out:
visit_end_struct(v, NULL);
}
--
Best regards,
Vladimir
[libvirt PATCH 3/4] qemu: assume QEMU_CAPS_FSDEV_CREATEMODE
Added by QEMU commit:
b96feb2cb9 "9pfs: local: Add support for custom fmode/dmode in 9ps
mapped security modes"
in 2.10.0
Signed-off-by: Ján Tomko
---
src/qemu/qemu_validate.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 7e4090181d..294cc24b36 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4165,11 +4165,6 @@ qemuValidateDomainDeviceDefFS(virDomainFSDef *fs,
}
if ((fs->fmode != 0) || (fs->dmode != 0)) {
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_CREATEMODE)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-_("fmode and dmode are not supported with this QEMU
binary"));
-return -1;
-}
if (fs->accessmode != VIR_DOMAIN_FS_ACCESSMODE_MAPPED) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("fmode and dmode must be used with
accessmode=mapped"));
--
2.31.1
[libvirt PATCH 4/4] qemu: deprecate QEMU_CAPS_FSDEV_CREATEMODE
Signed-off-by: Ján Tomko
---
src/qemu/qemu_capabilities.c | 3 +--
src/qemu/qemu_capabilities.h | 2 +-
tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml| 1 -
tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 -
41 files changed, 2 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index dc7e8f3c43..a1be0cb74e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -603,7 +603,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"virtio-balloon.free-page-reporting", /*
QEMU_CAPS_VIRTIO_BALLOON_FREE_PAGE_REPORTING */
"block-export-add", /* QEMU_CAPS_BLOCK_EXPORT_ADD */
"netdev.vhost-vdpa", /* QEMU_CAPS_NETDEV_VHOST_VDPA */
- "fsdev.createmode", /* QEMU_CAPS_FSDEV_CREATEMODE */
+ "fsdev.createmode", /* X_QEMU_CAPS_FSDEV_CREATEMODE */
/* 385 */
"ncr53c90", /* QEMU_CAPS_SCSI_NCR53C90 */
@@ -3204,7 +3204,6 @@ struct virQEMUCapsCommandLineProps {
static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = {
{ "chardev", "fd", QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE },
{ "fsdev", "multidevs", QEMU_CAPS_FSDEV_MULTIDEVS },
-{ "fsdev", "fmode", QEMU_CAPS_FSDEV_CREATEMODE }, /* Could have also
checked fsdev->dmode */
{ "numa", NULL, QEMU_CAPS_NUMA }, /* not needed after qemuCaps->version <
300 */
{ "overcommit", NULL, QEMU_CAPS_OVERCOMMIT },
{ "sandbox", NULL, QEMU_CAPS_SECCOMP_SANDBOX },
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 7c011812e6..b0fa1eec35 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -583,7 +583,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_VIRTIO_BALLOON_FREE_PAGE_REPORTING, /*virtio balloon
free-page-reporting */
QEMU_CAPS_BLOCK_EXPORT_ADD, /* 'block-export-add' command is supported */
QEMU_CAPS_NETDEV_VHOST_VDPA, /* -netdev vhost-vdpa*/
-QEMU_CAPS_FSDEV_CREATEMODE, /* fsdev.createmode */
+X_QEMU_CAPS_FSDEV_CREATEMODE, /* fsdev.createmode */
/* 385 */
QEMU_CAPS_SCSI_NCR53C90, /* built-in SCSI */
diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
index cb6510c238..e9fd6e96e5 100644
--- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
@@ -95,7 +95,6 @@
-
2011000
0
diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
index 7ad898a60e..d6549d6440 100644
--- a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
@@ -168,7 +168,6 @@
-
[libvirt PATCH 1/4] qemu: assume QEMU_CAPS_MACHINE_KERNEL_IRQCHIP
Even though we only allow this option on x86,
all QEMUs report the command line option.
Added in QEMU v1.1:
6a48ffaaa7 "kvm: Activate in-kernel irqchip support"
Remove the pointless capability.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_validate.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 13fbfd01b2..7e4090181d 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -195,13 +195,6 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
def->os.machine);
return -1;
}
-
-if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_MACHINE_KERNEL_IRQCHIP)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("I/O APIC tuning is not supported by "
- "this QEMU binary"));
-return -1;
-}
}
break;
--
2.31.1
[libvirt PATCH 2/4] qemu: Deprecate QEMU_CAPS_MACHINE_KERNEL_IRQCHIP
Now that it's no longer used, remove probing for it
and mark it as deprecated.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_capabilities.c | 3 +--
src/qemu/qemu_capabilities.h | 2 +-
tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml| 1 -
tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 -
41 files changed, 2 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index bfb59965e2..dc7e8f3c43 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -420,7 +420,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"query-named-block-nodes", /* QEMU_CAPS_QUERY_NAMED_BLOCK_NODES
*/
"cpu-cache", /* QEMU_CAPS_CPU_CACHE */
"qemu-xhci", /* QEMU_CAPS_DEVICE_QEMU_XHCI */
- "kernel-irqchip", /* QEMU_CAPS_MACHINE_KERNEL_IRQCHIP */
+ "kernel-irqchip", /* X_QEMU_CAPS_MACHINE_KERNEL_IRQCHIP */
"kernel-irqchip.split", /*
X_QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT */
/* 255 */
@@ -3205,7 +3205,6 @@ static struct virQEMUCapsCommandLineProps
virQEMUCapsCommandLine[] = {
{ "chardev", "fd", QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE },
{ "fsdev", "multidevs", QEMU_CAPS_FSDEV_MULTIDEVS },
{ "fsdev", "fmode", QEMU_CAPS_FSDEV_CREATEMODE }, /* Could have also
checked fsdev->dmode */
-{ "machine", "kernel_irqchip", QEMU_CAPS_MACHINE_KERNEL_IRQCHIP },
{ "numa", NULL, QEMU_CAPS_NUMA }, /* not needed after qemuCaps->version <
300 */
{ "overcommit", NULL, QEMU_CAPS_OVERCOMMIT },
{ "sandbox", NULL, QEMU_CAPS_SECCOMP_SANDBOX },
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 786c695880..7c011812e6 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -400,7 +400,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_QUERY_NAMED_BLOCK_NODES, /* qmp query-named-block-nodes */
QEMU_CAPS_CPU_CACHE, /* -cpu supports host-cache-info and l3-cache
properties */
QEMU_CAPS_DEVICE_QEMU_XHCI, /* -device qemu-xhci */
-QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, /* -machine kernel_irqchip */
+X_QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, /* -machine kernel_irqchip */
X_QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, /* -machine kernel_irqchip=split
*/
/* 255 */
diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
index 602b7ea10f..cb6510c238 100644
--- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
@@ -62,7 +62,6 @@
-
diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
index 8749f4ee64..7ad898a60e 100644
[libvirt PATCH 0/4] qemu: deprecate more capabilities
Applies on top of my seccomp sandbox series. Ján Tomko (4): qemu: assume QEMU_CAPS_MACHINE_KERNEL_IRQCHIP qemu: Deprecate QEMU_CAPS_MACHINE_KERNEL_IRQCHIP qemu: assume QEMU_CAPS_FSDEV_CREATEMODE qemu: deprecate QEMU_CAPS_FSDEV_CREATEMODE src/qemu/qemu_capabilities.c | 6 ++ src/qemu/qemu_capabilities.h | 4 ++-- src/qemu/qemu_validate.c | 12 tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 2 -- tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 2 -- tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 2 -- tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 2 -- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 2 -- tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 2 -- tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 2 -- tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml| 2 -- tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml| 2 -- tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 2 -- tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml| 2 -- tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 2 -- tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 2 -- tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml| 2 -- tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 2 -- tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml| 2 -- tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml| 2 -- tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 2 -- tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml| 2 -- tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 2 -- tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml| 2 -- tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 2 -- tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml| 2 -- tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 2 -- tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml| 2 -- tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 2 -- tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml| 2 -- tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 2 -- 42 files changed, 4 insertions(+), 96 deletions(-) -- 2.31.1
[PATCH v2 6/7] virsh: domain: remove 'ret' variable and use 'count' instead
This patch also includes use of an early return in case of an
error. I think the changes make the functions more readable.
Signed-off-by: Kristina Hanicova
---
tools/virsh-domain.c | 26 ++
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 3c496d845a..94ed786751 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -13608,10 +13608,10 @@ static bool
cmdDomFSFreeze(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
-int ret = -1;
const vshCmdOpt *opt = NULL;
g_autofree const char **mountpoints = NULL;
size_t nmountpoints = 0;
+int count = 0;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
@@ -13621,16 +13621,13 @@ cmdDomFSFreeze(vshControl *ctl, const vshCmd *cmd)
mountpoints[nmountpoints-1] = opt->data;
}
-ret = virDomainFSFreeze(dom, mountpoints, nmountpoints, 0);
-if (ret < 0) {
+if ((count = virDomainFSFreeze(dom, mountpoints, nmountpoints, 0)) < 0) {
vshError(ctl, _("Unable to freeze filesystems"));
-goto cleanup;
+return false;
}
-vshPrintExtra(ctl, _("Froze %d filesystem(s)\n"), ret);
-
- cleanup:
-return ret >= 0;
+vshPrintExtra(ctl, _("Froze %d filesystem(s)\n"), count);
+return true;
}
static const vshCmdInfo info_domfsthaw[] = {
@@ -13656,10 +13653,10 @@ static bool
cmdDomFSThaw(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
-int ret = -1;
const vshCmdOpt *opt = NULL;
g_autofree const char **mountpoints = NULL;
size_t nmountpoints = 0;
+int count = 0;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
@@ -13669,16 +13666,13 @@ cmdDomFSThaw(vshControl *ctl, const vshCmd *cmd)
mountpoints[nmountpoints-1] = opt->data;
}
-ret = virDomainFSThaw(dom, mountpoints, nmountpoints, 0);
-if (ret < 0) {
+if ((count = virDomainFSThaw(dom, mountpoints, nmountpoints, 0)) < 0) {
vshError(ctl, _("Unable to thaw filesystems"));
-goto cleanup;
+return false;
}
-vshPrintExtra(ctl, _("Thawed %d filesystem(s)\n"), ret);
-
- cleanup:
-return ret >= 0;
+vshPrintExtra(ctl, _("Thawed %d filesystem(s)\n"), count);
+return true;
}
static const vshCmdInfo info_domfsinfo[] = {
--
2.31.1
[PATCH v2 3/7] virsh: domain: remove 'ret' variable and use direct return when possible
Signed-off-by: Kristina Hanicova
---
tools/virsh-domain.c | 62
1 file changed, 22 insertions(+), 40 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0d482f9e15..1da9b183bf 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2702,7 +2702,6 @@ virshBlockJobAbort(virDomainPtr dom,
static bool
cmdBlockjob(vshControl *ctl, const vshCmd *cmd)
{
-bool ret = false;
bool raw = vshCommandOptBool(cmd, "raw");
bool bytes = vshCommandOptBool(cmd, "bytes");
bool abortMode = vshCommandOptBool(cmd, "abort");
@@ -2735,13 +2734,10 @@ cmdBlockjob(vshControl *ctl, const vshCmd *cmd)
return false;
if (bandwidth)
-ret = virshBlockJobSetSpeed(ctl, cmd, dom, path, bytes);
-else if (abortMode || pivot || async)
-ret = virshBlockJobAbort(dom, path, pivot, async);
-else
-ret = virshBlockJobInfo(ctl, dom, path, raw, bytes);
-
-return ret;
+return virshBlockJobSetSpeed(ctl, cmd, dom, path, bytes);
+if (abortMode || pivot || async)
+return virshBlockJobAbort(dom, path, pivot, async);
+return virshBlockJobInfo(ctl, dom, path, raw, bytes);
}
/*
@@ -5063,7 +5059,6 @@ cmdSchedInfoUpdateOne(vshControl *ctl,
const char *field, const char *value)
{
virTypedParameterPtr param;
-int ret = -1;
size_t i;
for (i = 0; i < nsrc_params; i++) {
@@ -5078,14 +5073,11 @@ cmdSchedInfoUpdateOne(vshControl *ctl,
vshSaveLibvirtError();
return -1;
}
-ret = 0;
-break;
+return 0;
}
-if (ret < 0)
-vshError(ctl, _("invalid scheduler option: %s"), field);
-
-return ret;
+vshError(ctl, _("invalid scheduler option: %s"), field);
+return -1;
}
static int
@@ -5529,7 +5521,6 @@ virshGenFileName(vshControl *ctl, virDomainPtr dom, const
char *mime)
g_autoptr(GDateTime) now = g_date_time_new_now_local();
g_autofree char *nowstr = NULL;
const char *ext = NULL;
-char *ret = NULL;
if (!dom) {
vshError(ctl, "%s", _("Invalid domain supplied"));
@@ -5544,10 +5535,8 @@ virshGenFileName(vshControl *ctl, virDomainPtr dom,
const char *mime)
nowstr = g_date_time_format(now, "%Y-%m-%d-%H:%M:%S");
-ret = g_strdup_printf("%s-%s%s", virDomainGetName(dom),
- nowstr, NULLSTR_EMPTY(ext));
-
-return ret;
+return g_strdup_printf("%s-%s%s", virDomainGetName(dom),
+ nowstr, NULLSTR_EMPTY(ext));
}
static bool
@@ -5686,7 +5675,6 @@ static bool
cmdSetLifecycleAction(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
-bool ret = true;
bool config = vshCommandOptBool(cmd, "config");
bool live = vshCommandOptBool(cmd, "live");
bool current = vshCommandOptBool(cmd, "current");
@@ -5727,10 +5715,9 @@ cmdSetLifecycleAction(vshControl *ctl, const vshCmd *cmd)
if (virDomainSetLifecycleAction(dom, type, action, flags) < 0) {
vshError(ctl, "%s", _("Unable to change lifecycle action."));
-ret = false;
+return false;
}
-
-return ret;
+return true;
}
/*
@@ -6469,15 +6456,14 @@ static bool
cmdDomjobabort(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
-bool ret = true;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
if (virDomainAbortJob(dom) < 0)
-ret = false;
+return false;
-return ret;
+return true;
}
/*
@@ -11926,12 +11912,12 @@ virshDomainDetachInterface(char *doc,
xmlNodePtr cur = NULL, matchNode = NULL;
g_autofree char *detach_xml = NULL;
char buf[64];
-int diff_mac, ret = -1;
+int diff_mac = -1;
size_t i;
if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) {
vshError(ctl, "%s", _("Failed to get interface information"));
-goto cleanup;
+return false;
}
g_snprintf(buf, sizeof(buf), "/domain/devices/interface[@type='%s']",
type);
@@ -11939,13 +11925,13 @@ virshDomainDetachInterface(char *doc,
if (obj == NULL || obj->type != XPATH_NODESET ||
obj->nodesetval == NULL || obj->nodesetval->nodeNr == 0) {
vshError(ctl, _("No interface found whose type is %s"), type);
-goto cleanup;
+return false;
}
if (!mac && obj->nodesetval->nodeNr > 1) {
vshError(ctl, _("Domain has %d interfaces. Please specify which one "
"to detach using --mac"), obj->nodesetval->nodeNr);
-goto cleanup;
+return false;
}
if (!mac) {
@@ -11968,7 +11954,7 @@ virshDomainDetachInterface(char *doc,
"MAC address %s. You must use
detach-device and "
"specify the device pci address to
remove it."),
[PATCH v2 7/7] virsh: domain: remove else branch
I removed else branches after return/break as they are not
necessary and the code looks cleaner without them.
Signed-off-by: Kristina Hanicova
---
tools/virsh-domain.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 94ed786751..ca8f08d949 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11046,12 +11046,12 @@ cmdMigrateCompCache(vshControl *ctl, const vshCmd
*cmd)
return false;
rc = vshCommandOptULongLong(ctl, cmd, "size", &size);
-if (rc < 0) {
+if (rc < 0)
+return false;
+
+if (rc != 0 &&
+(virDomainMigrateSetCompressionCache(dom, size, 0) < 0))
return false;
-} else if (rc != 0) {
-if (virDomainMigrateSetCompressionCache(dom, size, 0) < 0)
-return false;
-}
if (virDomainMigrateGetCompressionCache(dom, &size, 0) < 0)
return false;
@@ -11432,11 +11432,9 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
/* We got what we came for so return successfully */
ret = true;
-if (!all) {
+if (!all)
break;
-} else {
-vshPrint(ctl, "\n");
-}
+vshPrint(ctl, "\n");
}
if (!ret) {
--
2.31.1
[PATCH v2 5/7] virsh: domain: use early return when possible
Signed-off-by: Kristina Hanicova
---
tools/virsh-domain.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1692197d1a..3c496d845a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5883,13 +5883,13 @@ cmdShutdown(vshControl *ctl, const vshCmd *cmd)
rv = virDomainShutdownFlags(dom, flags);
else
rv = virDomainShutdown(dom);
-if (rv == 0) {
-vshPrintExtra(ctl, _("Domain '%s' is being shutdown\n"), name);
-} else {
+
+if (rv != 0) {
vshError(ctl, _("Failed to shutdown domain '%s'"), name);
return false;
}
+vshPrintExtra(ctl, _("Domain '%s' is being shutdown\n"), name);
return true;
}
@@ -5959,13 +5959,12 @@ cmdReboot(vshControl *ctl, const vshCmd *cmd)
if (!(dom = virshCommandOptDomain(ctl, cmd, &name)))
return false;
-if (virDomainReboot(dom, flags) == 0) {
-vshPrintExtra(ctl, _("Domain '%s' is being rebooted\n"), name);
-} else {
+if (virDomainReboot(dom, flags) != 0) {
vshError(ctl, _("Failed to reboot domain '%s'"), name);
return false;
}
+vshPrintExtra(ctl, _("Domain '%s' is being rebooted\n"), name);
return true;
}
--
2.31.1
[PATCH v2 4/7] virsh: domain: remove 'ret' variable, use early return when possible
Signed-off-by: Kristina Hanicova
---
tools/virsh-domain.c | 84 +---
1 file changed, 33 insertions(+), 51 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1da9b183bf..1692197d1a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2923,7 +2923,6 @@ cmdBlockresize(vshControl *ctl, const vshCmd *cmd)
const char *path = NULL;
unsigned long long size = 0;
unsigned int flags = 0;
-bool ret = false;
if (vshCommandOptStringReq(ctl, cmd, "path", (const char **) &path) < 0)
return false;
@@ -2942,12 +2941,11 @@ cmdBlockresize(vshControl *ctl, const vshCmd *cmd)
if (virDomainBlockResize(dom, path, size, flags) < 0) {
vshError(ctl, _("Failed to resize block device '%s'"), path);
-} else {
-vshPrintExtra(ctl, _("Block device '%s' is resized"), path);
-ret = true;
+return false;
}
-return ret;
+vshPrintExtra(ctl, _("Block device '%s' is resized"), path);
+return true;
}
#ifndef WIN32
@@ -3421,19 +3419,17 @@ cmdSuspend(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
const char *name;
-bool ret = true;
if (!(dom = virshCommandOptDomain(ctl, cmd, &name)))
return false;
-if (virDomainSuspend(dom) == 0) {
-vshPrintExtra(ctl, _("Domain '%s' suspended\n"), name);
-} else {
+if (virDomainSuspend(dom) != 0) {
vshError(ctl, _("Failed to suspend domain '%s'"), name);
-ret = false;
+return false;
}
-return ret;
+vshPrintExtra(ctl, _("Domain '%s' suspended\n"), name);
+return true;
}
/*
@@ -5802,20 +5798,18 @@ static bool
cmdResume(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
-bool ret = true;
const char *name;
if (!(dom = virshCommandOptDomain(ctl, cmd, &name)))
return false;
-if (virDomainResume(dom) == 0) {
-vshPrintExtra(ctl, _("Domain '%s' resumed\n"), name);
-} else {
+if (virDomainResume(dom) != 0) {
vshError(ctl, _("Failed to resume domain '%s'"), name);
-ret = false;
+return false;
}
-return ret;
+vshPrintExtra(ctl, _("Domain '%s' resumed\n"), name);
+return true;
}
/*
@@ -5997,20 +5991,18 @@ static bool
cmdReset(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
-bool ret = true;
const char *name;
if (!(dom = virshCommandOptDomain(ctl, cmd, &name)))
return false;
-if (virDomainReset(dom, 0) == 0) {
-vshPrintExtra(ctl, _("Domain '%s' was reset\n"), name);
-} else {
+if (virDomainReset(dom, 0) != 0) {
vshError(ctl, _("Failed to reset domain '%s'"), name);
-ret = false;
+return false;
}
-return ret;
+vshPrintExtra(ctl, _("Domain '%s' was reset\n"), name);
+return true;
}
/*
@@ -8183,7 +8175,6 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
const char *from = NULL;
-bool ret = true;
char *buffer;
unsigned int flags = 0;
virshControl *priv = ctl->privData;
@@ -8203,14 +8194,14 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd)
dom = virDomainDefineXML(priv->conn, buffer);
VIR_FREE(buffer);
-if (dom != NULL) {
-vshPrintExtra(ctl, _("Domain '%s' defined from %s\n"),
- virDomainGetName(dom), from);
-} else {
+if (!dom) {
vshError(ctl, _("Failed to define domain from %s"), from);
-ret = false;
+return false;
}
-return ret;
+
+vshPrintExtra(ctl, _("Domain '%s' defined from %s\n"),
+ virDomainGetName(dom), from);
+return true;
}
/*
@@ -8239,7 +8230,6 @@ static bool
cmdDestroy(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
-bool ret = true;
const char *name;
unsigned int flags = 0;
int result;
@@ -8255,14 +8245,13 @@ cmdDestroy(vshControl *ctl, const vshCmd *cmd)
else
result = virDomainDestroy(dom);
-if (result == 0) {
-vshPrintExtra(ctl, _("Domain '%s' destroyed\n"), name);
-} else {
+if (result < 0) {
vshError(ctl, _("Failed to destroy domain '%s'"), name);
-ret = false;
+return false;
}
-return ret;
+vshPrintExtra(ctl, _("Domain '%s' destroyed\n"), name);
+return true;
}
/*
@@ -9955,7 +9944,6 @@ static bool
cmdDumpXML(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
-bool ret = true;
g_autofree char *dump = NULL;
unsigned int flags = 0;
bool inactive = vshCommandOptBool(cmd, "inactive");
@@ -9975,14 +9963,11 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd)
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
-dump = virDomainGetXMLDesc(dom, flags);
-if (dump != NULL) {
-
[PATCH v2 2/7] virsh: domain: remove nested 'if' in cmdAttachDisk()
Signed-off-by: Kristina Hanicova
---
tools/virsh-domain.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index edee548d8a..0d482f9e15 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -659,12 +659,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
return false;
}
-if (mode) {
-if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) {
-vshError(ctl, _("No support for %s in command 'attach-disk'"),
- mode);
-return false;
-}
+if (mode && STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) {
+vshError(ctl, _("No support for %s in command 'attach-disk'"), mode);
+return false;
}
if (wwn && !virValidateWWN(wwn))
--
2.31.1
[PATCH v2 1/7] virsh: domain: use early return in virshDomainDefine()
Signed-off-by: Kristina Hanicova
---
tools/virsh-domain.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index bee3346eb0..edee548d8a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -245,18 +245,18 @@ static virDomainPtr
virshDomainDefine(virConnectPtr conn, const char *xml, unsigned int flags)
{
virDomainPtr dom;
-if (flags) {
-dom = virDomainDefineXMLFlags(conn, xml, flags);
-/* If validate is the only flag, just drop it and
- * try again.
- */
-if (!dom) {
-if ((virGetLastErrorCode() == VIR_ERR_NO_SUPPORT) &&
-(flags == VIR_DOMAIN_DEFINE_VALIDATE))
-dom = virDomainDefineXML(conn, xml);
-}
-} else {
-dom = virDomainDefineXML(conn, xml);
+
+if (!flags)
+return virDomainDefineXML(conn, xml);
+
+dom = virDomainDefineXMLFlags(conn, xml, flags);
+/* If validate is the only flag, just drop it and
+ * try again.
+ */
+if (!dom) {
+if ((virGetLastErrorCode() == VIR_ERR_NO_SUPPORT) &&
+(flags == VIR_DOMAIN_DEFINE_VALIDATE))
+dom = virDomainDefineXML(conn, xml);
}
return dom;
}
--
2.31.1
[PATCH v2 0/7] virsh: domain: small refactoring
This is partially v2 of: https://listman.redhat.com/archives/libvir-list/2021-September/msg00728.html Diff to v1: * split changes into smaller patches Kristina Hanicova (7): virsh: domain: use early return in virshDomainDefine() virsh: domain: remove nested 'if' in cmdAttachDisk() virsh: domain: remove 'ret' variable and use direct return when possible virsh: domain: remove 'ret' variable, use early return when possible virsh: domain: use early return when possible virsh: domain: remove 'ret' variable and use 'count' instead virsh: domain: remove else branch tools/virsh-domain.c | 232 +-- 1 file changed, 92 insertions(+), 140 deletions(-) -- 2.31.1
Re: [PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts
Am 24.09.2021 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.09.2021 12:04, Kevin Wolf wrote:
> > DeviceState.id is a pointer to a string that is stored in the QemuOpts
> > object DeviceState.opts and freed together with it. We want to create
> > devices without going through QemuOpts in the future, so make this a
> > separately allocated string.
> >
> > Signed-off-by: Kevin Wolf
>
> Interesting that in hw/xen/xen-legacy-backend.c
> g_strdup_printf-allocated id is passed to qdev_set_id prior this
> patch. So, the patch seems to fix that small leak. Worth to mention?
Ok, I can mention it explicitly.
> > ---
> > include/hw/qdev-core.h | 2 +-
> > include/monitor/qdev.h | 2 +-
> > hw/arm/virt.c | 2 +-
> > hw/core/qdev.c | 1 +
> > hw/pci-bridge/pci_expander_bridge.c | 2 +-
> > hw/ppc/e500.c | 2 +-
> > softmmu/qdev-monitor.c | 5 +++--
> > 7 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 34c8a7506a..1857d9698e 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -176,7 +176,7 @@ struct DeviceState {
> > Object parent_obj;
> > /*< public >*/
> > -const char *id;
> > +char *id;
> > char *canonical_path;
> > bool realized;
> > bool pending_deleted_event;
> > diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> > index eaa947d73a..389287eb44 100644
> > --- a/include/monitor/qdev.h
> > +++ b/include/monitor/qdev.h
> > @@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
> > **errp);
> > int qdev_device_help(QemuOpts *opts);
> > DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
> > -void qdev_set_id(DeviceState *dev, const char *id);
> > +void qdev_set_id(DeviceState *dev, char *id);
> > #endif
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 1d59f0e59f..f933d48d3b 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
> > MemoryRegion *sysmem = get_system_memory();
> > dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
> > -dev->id = TYPE_PLATFORM_BUS_DEVICE;
> > +dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> > qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
> > qdev_prop_set_uint32(dev, "mmio_size",
> > vms->memmap[VIRT_PLATFORM_BUS].size);
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index cefc5eaa0a..d918b50a1d 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
> > }
> > qemu_opts_del(dev->opts);
> > +g_free(dev->id);
> > }
> > static void device_class_base_init(ObjectClass *class, void *data)
> > diff --git a/hw/pci-bridge/pci_expander_bridge.c
> > b/hw/pci-bridge/pci_expander_bridge.c
> > index 7112dc3062..10e6e7c2ab 100644
> > --- a/hw/pci-bridge/pci_expander_bridge.c
> > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > @@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool
> > pcie, Error **errp)
> > } else {
> > bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
> > TYPE_PXB_BUS);
> > bds = qdev_new("pci-bridge");
> > -bds->id = dev_name;
> > +bds->id = g_strdup(dev_name);
> > qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR,
> > pxb->bus_nr);
> > qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
> > }
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > index 95451414dd..960e7efcd3 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
> > /* Platform Bus Device */
> > if (pmc->has_platform_bus) {
> > dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
> > -dev->id = TYPE_PLATFORM_BUS_DEVICE;
> > +dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> > qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
> > qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > index 034b999401..1207e57a46 100644
> > --- a/softmmu/qdev-monitor.c
> > +++ b/softmmu/qdev-monitor.c
> > @@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error
> > **errp)
> > return bus;
> > }
> > -void qdev_set_id(DeviceState *dev, const char *id)
> > +/* Takes ownership of @id, will be freed when deleting the device */
> > +void qdev_set_id(DeviceState *dev, char *id)
> > {
> > if (id) {
> > dev->id = id;
> > @@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error
> > **errp)
> > }
Re: [PATCH v3 0/3] remove sysconfig files
Am Fri, 24 Sep 2021 08:49:55 -0600 schrieb Jim Fehlig : > Needs rebased again :-). Sadly, yes... Olaf pgp2jGPfmD95H.pgp Description: Digitale Signatur von OpenPGP
[libvirt PATCH 5/5] qemu: capabilities: do not look at parameters for sandbox
Assume the presence of the 'sandbox' option is enough,
no need to look at the parameters.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_capabilities.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4f18a2488a..bfb59965e2 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3208,7 +3208,7 @@ static struct virQEMUCapsCommandLineProps
virQEMUCapsCommandLine[] = {
{ "machine", "kernel_irqchip", QEMU_CAPS_MACHINE_KERNEL_IRQCHIP },
{ "numa", NULL, QEMU_CAPS_NUMA }, /* not needed after qemuCaps->version <
300 */
{ "overcommit", NULL, QEMU_CAPS_OVERCOMMIT },
-{ "sandbox", "enable", QEMU_CAPS_SECCOMP_SANDBOX },
+{ "sandbox", NULL, QEMU_CAPS_SECCOMP_SANDBOX },
{ "spice", "gl", QEMU_CAPS_SPICE_GL },
{ "spice", "unix", QEMU_CAPS_SPICE_UNIX },
{ "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE },
--
2.31.1
[libvirt PATCH 2/5] qemu: conf: simplify seccomp_sandbox comment
It contains too many negations and conditions that are no longer relevant now that we only support QEMU >= 2.11. Signed-off-by: Ján Tomko --- src/qemu/qemu.conf | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 8722dc169c..71fd125699 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -769,13 +769,12 @@ -# Use seccomp syscall sandbox in QEMU. -# 1 == seccomp enabled, 0 == seccomp disabled +# Use seccomp syscall filtering sandbox in QEMU. +# 1 == filter enabled, 0 == filter disabled # -# If it is unset (or -1), then seccomp will be enabled -# only if QEMU >= 2.11.0 is detected, otherwise it is -# left disabled. This ensures the default config gets -# protection for new QEMU using the blacklist approach. +# Unless this option is disabled, QEMU will be run with +# a seccomp filter that stops it from executing certain +# syscalls. # #seccomp_sandbox = 1 -- 2.31.1
[libvirt PATCH 4/5] qemu: capabilities: deprecate QEMU_CAPS_SECCOMP_BLACKLIST
Signed-off-by: Ján Tomko
---
src/qemu/qemu_capabilities.c | 3 +--
src/qemu/qemu_capabilities.h | 2 +-
tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml| 1 -
tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 -
41 files changed, 2 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index db5432c9fc..4f18a2488a 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -468,7 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 285 */
"qcow2-luks", /* QEMU_CAPS_QCOW2_LUKS */
"pcie-pci-bridge", /* QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE */
- "seccomp-blacklist", /* QEMU_CAPS_SECCOMP_BLACKLIST */
+ "seccomp-blacklist", /* X_QEMU_CAPS_SECCOMP_BLACKLIST */
"query-cpus-fast", /* QEMU_CAPS_QUERY_CPUS_FAST */
"disk-write-cache", /* QEMU_CAPS_DISK_WRITE_CACHE */
@@ -3209,7 +3209,6 @@ static struct virQEMUCapsCommandLineProps
virQEMUCapsCommandLine[] = {
{ "numa", NULL, QEMU_CAPS_NUMA }, /* not needed after qemuCaps->version <
300 */
{ "overcommit", NULL, QEMU_CAPS_OVERCOMMIT },
{ "sandbox", "enable", QEMU_CAPS_SECCOMP_SANDBOX },
-{ "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST },
{ "spice", "gl", QEMU_CAPS_SPICE_GL },
{ "spice", "unix", QEMU_CAPS_SPICE_UNIX },
{ "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE },
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 097f28bd40..786c695880 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -448,7 +448,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
/* 285 */
QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */
QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */
-QEMU_CAPS_SECCOMP_BLACKLIST, /* -sandbox.elevateprivileges */
+X_QEMU_CAPS_SECCOMP_BLACKLIST, /* -sandbox.elevateprivileges */
QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */
QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache
param */
diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
index f2a89d5c58..602b7ea10f 100644
--- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
@@ -79,7 +79,6 @@
-
diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
index 1e08a04c82..8749f4ee64 100644
--- a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
@@ -149,7 +149,6 @@
-
diff --git a/tests/qemucapabilit
[libvirt PATCH 3/5] qemu: seccomp: remove dead code
There is no QEMU we support that would need the old syntax
for -sandbox on.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_command.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fa9998a191..48df8818a6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10119,7 +10119,6 @@ qemuBuildSeccompSandboxCommandLine(virCommand *cmd,
return 0;
}
-/* Use blacklist by default if supported */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) {
virCommandAddArgList(cmd, "-sandbox",
"on,obsolete=deny,elevateprivileges=deny,"
@@ -10128,10 +10127,6 @@ qemuBuildSeccompSandboxCommandLine(virCommand *cmd,
return 0;
}
-/* Seccomp whitelist is opt-in */
-if (cfg->seccompSandbox > 0)
-virCommandAddArgList(cmd, "-sandbox", "on", NULL);
-
return 0;
}
--
2.31.1
[libvirt PATCH 0/5] qemu: simplify seccomp sandbox
Inspired by Peter's q-c-l-o cleanups. Ján Tomko (5): qemu: always assume QEMU_CAPS_SECCOMP_BLACKLIST qemu: conf: simplify seccomp_sandbox comment qemu: seccomp: remove dead code qemu: capabilities: deprecate QEMU_CAPS_SECCOMP_BLACKLIST qemu: capabilities: do not look at parameters for sandbox src/qemu/qemu.conf | 11 +-- src/qemu/qemu_capabilities.c | 5 ++--- src/qemu/qemu_capabilities.h | 2 +- src/qemu/qemu_command.c| 7 +-- tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 - tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 - tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 - tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 - tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 - tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 - tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 - tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml| 1 - tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml| 1 - tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 - tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml| 1 - tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 - tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 - tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml| 1 - tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 1 - tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml| 1 - tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml| 1 - tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 - tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml| 1 - tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 - tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml| 1 - tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 - tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml| 1 - tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 1 - tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml| 1 - tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 - tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml| 1 - tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 - 43 files changed, 9 insertions(+), 55 deletions(-) -- 2.31.1
[libvirt PATCH 1/5] qemu: always assume QEMU_CAPS_SECCOMP_BLACKLIST
elevateprivileges was introduced by QEMU commit:
73a1e64725 "seccomp: add elevateprivileges argument to command line"
released in 2.11.0
and later made conditional on SECCOMP support by:
9d0fdecbad sandbox: disable -sandbox if CONFIG_SECCOMP undefined
Use the existence of the sandbox option as a witness for its support.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_command.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b60ee1192b..fa9998a191 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10120,7 +10120,7 @@ qemuBuildSeccompSandboxCommandLine(virCommand *cmd,
}
/* Use blacklist by default if supported */
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) {
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) {
virCommandAddArgList(cmd, "-sandbox",
"on,obsolete=deny,elevateprivileges=deny,"
"spawn=deny,resourcecontrol=deny",
--
2.31.1
Re: [PATCH 01/11] qom: Reduce use of error_propagate()
Kevin Wolf writes: > ERRP_GUARD() makes debugging easier by making sure that &error_abort > still fails at the real origin of the error instead of > error_propagate(). > > Signed-off-by: Kevin Wolf Yes. The code you patch uses error_propagate() to work around functions not returning distinct error values. error.h's big comment recommends such return values, but recommendations don't update code, patches do. Until then, ERRP_GUARD() is clearly a better crutch than error_propagate(). Reviewed-by: Markus Armbruster
Re: [PATCH v2 1/2] virsh: domain: fix mistake in cmdMigrateSetMaxDowntime()
On Fri, Sep 24, 2021 at 11:03 AM Michal Prívozník wrote: > On 9/24/21 1:30 AM, Kristina Hanicova wrote: > > Function returned false when everything ended successfully, there > > was a missing check for a return value. This patch fixes that. > > > > Signed-off-by: Kristina Hanicova > > --- > > tools/virsh-domain.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > > index f876f30cc5..2730acfba5 100644 > > --- a/tools/virsh-domain.c > > +++ b/tools/virsh-domain.c > > @@ -11018,7 +11018,7 @@ cmdMigrateSetMaxDowntime(vshControl *ctl, const > vshCmd *cmd) > > goto done; > > } > > > > -if (virDomainMigrateSetMaxDowntime(dom, downtime, 0)) > > +if (virDomainMigrateSetMaxDowntime(dom, downtime, 0) < 0) > > goto done; > > > > ret = true; > > > > Technically, there wasn't any bug here. virDomainMigrateSetMaxDowntime() > returns 0 on success and 0 won't evaluate to true thus 'goto done' is > skipped over and ret is set to true. > > And if anything else is returned (currently only -1 is possible) then > i's evaluated to true and control jump to the done label. BUT, your fix > is correct because in libvirt only negative values are treated as > errors. Zero and positive values are treated as success. I'll reword the > commit message a bit. > > Sorry for not realizing this earlier. > > Michal > > I think the commit message could be rewritten to something as: If there was added a new return value indicating success to the function virDomainMigrateSetMaxDowntime() in the future, our function would treat it as an error state and would return false (indicating failure). This patch fixes it, so that the call of the function follows the same pattern as is currently set in libvirt.
Re: [PATCH] vsh: Don't check for OOM in vshGetTypedParamValue()
On a Friday in 2021, Michal Privoznik wrote: Both function description and function itself mention check for OOM which can't happen really. There was a bug in glib where g_strdup_*() might have not aborted on OOM, but we have our own implementation when dealing with broken glib (see vir_g_strdup_printf()). Therefore, checking for OOM is redundant and can never be true. Signed-off-by: Michal Privoznik --- tools/vsh.c | 25 + 1 file changed, 9 insertions(+), 16 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 1/6] virsh: domain: refactor cmdSchedinfo()
On a Friday in 2021, Michal Prívozník wrote:
On 9/24/21 1:25 AM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova
---
tools/virsh-domain.c | 99 +---
1 file changed, 47 insertions(+), 52 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index f876f30cc5..b64df640ba 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5157,7 +5157,6 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
int nparams = 0;
int nupdates = 0;
size_t i;
-int ret;
bool ret_val = false;
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
bool current = vshCommandOptBool(cmd, "current");
@@ -5176,73 +5175,69 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
return false;
/* Print SchedulerType */
-schedulertype = virDomainGetSchedulerType(dom, &nparams);
-if (schedulertype != NULL) {
-vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype);
-VIR_FREE(schedulertype);
-} else {
+if (!(schedulertype = virDomainGetSchedulerType(dom, &nparams))) {
vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), _("Unknown"));
goto cleanup;
}
-if (nparams) {
-params = g_new0(virTypedParameter, nparams);
+vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype);
+VIR_FREE(schedulertype);
I think instead of adding this VIR_FREE() you can just mark the variable
as g_autofree. It's a negligible change compared how much lines are
being changed.
Which sounds like a good reason not to mix it with the code movement.
Jano
signature.asc
Description: PGP signature
[PATCH] vsh: Don't check for OOM in vshGetTypedParamValue()
Both function description and function itself mention check for
OOM which can't happen really. There was a bug in glib where
g_strdup_*() might have not aborted on OOM, but we have our own
implementation when dealing with broken glib (see
vir_g_strdup_printf()). Therefore, checking for OOM is redundant
and can never be true.
Signed-off-by: Michal Privoznik
---
tools/vsh.c | 25 +
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index e81369f224..189c452f73 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -1806,51 +1806,44 @@ vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd
*cmd, int *timeout)
* ---
*/
-/* Return a non-NULL string representation of a typed parameter; exit
- * if we are out of memory. */
+/* Return a non-NULL string representation of a typed parameter; exit on
+ * unknown type. */
char *
vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item)
{
-char *str = NULL;
-
switch (item->type) {
case VIR_TYPED_PARAM_INT:
-str = g_strdup_printf("%d", item->value.i);
+return g_strdup_printf("%d", item->value.i);
break;
case VIR_TYPED_PARAM_UINT:
-str = g_strdup_printf("%u", item->value.ui);
+return g_strdup_printf("%u", item->value.ui);
break;
case VIR_TYPED_PARAM_LLONG:
-str = g_strdup_printf("%lld", item->value.l);
+return g_strdup_printf("%lld", item->value.l);
break;
case VIR_TYPED_PARAM_ULLONG:
-str = g_strdup_printf("%llu", item->value.ul);
+return g_strdup_printf("%llu", item->value.ul);
break;
case VIR_TYPED_PARAM_DOUBLE:
-str = g_strdup_printf("%f", item->value.d);
+return g_strdup_printf("%f", item->value.d);
break;
case VIR_TYPED_PARAM_BOOLEAN:
-str = g_strdup(item->value.b ? _("yes") : _("no"));
+return g_strdup(item->value.b ? _("yes") : _("no"));
break;
case VIR_TYPED_PARAM_STRING:
-str = g_strdup(item->value.s);
+return g_strdup(item->value.s);
break;
default:
vshError(ctl, _("unimplemented parameter type %d"), item->type);
-}
-
-if (!str) {
-vshError(ctl, "%s", _("Out of memory"));
exit(EXIT_FAILURE);
}
-return str;
}
void
--
2.32.0
Re: [libvirt PATCH] meson: Increase stack size limit for sanitizer builds
On Thu, Sep 23, 2021 at 11:23:38AM +0200, Tim Wiederhake wrote:
> When building with "CC=clang", "-Db_sanitize=address,undefined", and
> "-Dbuildtype=debug", the following error occurs:
>
> ../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of 10616
> bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe-larger-than=]
> virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
> ^
> 1 error generated.
>
> Enforcing stack frame only makes sense on normal builds when stack usage
> is deterministic.
>
> Signed-off-by: Tim Wiederhake
> ---
> meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 488490181f..be5a99a069 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -225,7 +225,7 @@ alloc_max = run_command(
> )
>
> # sanitizer instrumentation may enlarge stack frames
> -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 8192
> +stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768
>
> # array_bounds=2 check triggers false positive on some GCC
> # versions when using sanitizers. Seen on Fedora 34 with
Reviewed-by: Daniel P. Berrangé
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 :|
[PATCH 03/11] iotests/051: Fix typo
The iothread isn't called 'iothread0', but 'thread0'. Depending on the order that properties are parsed, the error message may change from the expected one to another one saying that the iothread doesn't exist. Signed-off-by: Kevin Wolf --- tests/qemu-iotests/051| 2 +- tests/qemu-iotests/051.pc.out | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index 7bf29343d7..1d2fa93a11 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -199,7 +199,7 @@ case "$QEMU_DEFAULT_MACHINE" in # virtio-blk enables the iothread only when the driver initialises the # device, so a second virtio-blk device can't be added even with the # same iothread. virtio-scsi allows this. -run_qemu $iothread -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on +run_qemu $iothread -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on run_qemu $iothread -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on ;; *) diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index afe7632964..063e4fc584 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -183,9 +183,9 @@ Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id QEMU X.Y.Z monitor - type 'help' for more information (qemu) QEMU_PROG: -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on: Cannot change iothread of active block backend -Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on +Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on: Cannot change iothread of active block backend +(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -- 2.31.1
[PATCH 00/11] qdev: Add JSON -device and fix QMP device_add
It's still a long way until we'll have QAPIfied devices, but there are some improvements that we can already make now to make the future switch easier. One important part of this is having code paths without QemuOpts, which we want to get rid of and replace with the keyval parser in the long run. This series adds support for JSON syntax to -device, which bypasses QemuOpts. While we're not using QAPI yet, devices are based on QOM, so we already do have type checks and an implied schema. JSON syntax supported now can be supported by QAPI later and regarding command line compatibility, actually switching to it becomes an implementation detail this way (of course, it will still add valuable user-visible features like introspection and documentation). Apart from making things more future proof, this also immediately adds a way to do non-scalar properties on the command line. nvme could have used list support recently, and the lack of it in -device led to some rather unnatural solution in the first version (doing the relationship between a device and objects backwards) and loss of features in the following. With this series, using a list as a device property should be possible without any weird tricks. Unfortunately, even QMP device_add goes through QemuOpts before this series, which destroys any type safety QOM provides and also can't support non-scalar properties. This is a bug that is fixed during this series, but that is technically an incompatible change. A similar change was made for netdev_add in commit db2a380c84. libvirt needs to make sure that it actually always passes the right type, because passing a wrong type will start to fail after this series. I hope that libvirt already does things correctly, so this won't cause any trouble, but if it does, there is the option of using the QemuOpts-less code path only for JSON -device and going through a deprecation period for QMP device_add. Kevin Wolf (11): qom: Reduce use of error_propagate() iotests/245: Fix type for iothread property iotests/051: Fix typo qdev: Avoid using string visitor for properties qdev: Make DeviceState.id independent of QemuOpts qdev: Add Error parameter to qdev_set_id() qemu-option: Allow deleting opts during qemu_opts_foreach() qdev: Base object creation on QDict rather than QemuOpts qdev: Avoid QemuOpts in QMP device_add vl: Enable JSON syntax for -device Deprecate stable non-JSON -device and -object qapi/qdev.json | 15 +++-- docs/about/deprecated.rst | 11 include/hw/qdev-core.h | 10 ++-- include/monitor/qdev.h | 2 +- hw/arm/virt.c | 2 +- hw/core/qdev.c | 6 +- hw/net/virtio-net.c | 4 +- hw/pci-bridge/pci_expander_bridge.c | 2 +- hw/ppc/e500.c | 2 +- hw/vfio/pci.c | 4 +- hw/xen/xen-legacy-backend.c | 3 +- qom/object.c| 7 +-- qom/object_interfaces.c | 17 ++ softmmu/qdev-monitor.c | 90 + softmmu/vl.c| 58 --- util/qemu-option.c | 4 +- tests/qemu-iotests/051 | 2 +- tests/qemu-iotests/051.pc.out | 4 +- tests/qemu-iotests/245 | 4 +- 19 files changed, 161 insertions(+), 86 deletions(-) -- 2.31.1
[PATCH 06/11] qdev: Add Error parameter to qdev_set_id()
object_property_add_child() fails (with &error_abort) if an object with
the same name already exists. As long as QemuOpts is in use for -device
and device_add, it catches duplicate IDs before qdev_set_id() is even
called. However, for enabling non-QemuOpts code paths, we need to make
sure that the condition doesn't cause a crash, but fails gracefully.
Signed-off-by: Kevin Wolf
---
include/monitor/qdev.h | 2 +-
hw/xen/xen-legacy-backend.c | 3 ++-
softmmu/qdev-monitor.c | 16 ++--
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..7961308c75 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+void qdev_set_id(DeviceState *dev, char *id, Error **errp);
#endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..17aca85ddc 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char
*type, int dom,
xendev = g_malloc0(ops->size);
object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
OBJECT(xendev)->free = g_free;
-qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
+qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+&error_abort);
qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal);
object_unref(OBJECT(xendev));
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..c2af906df0 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, Error **errp)
}
/* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+void qdev_set_id(DeviceState *dev, char *id, Error **errp)
{
if (id) {
dev->id = id;
}
if (dev->id) {
-object_property_add_child(qdev_get_peripheral(), dev->id,
- OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral(), dev->id,
+ OBJECT(dev), errp);
} else {
static int anon_count;
gchar *name = g_strdup_printf("device[%d]", anon_count++);
-object_property_add_child(qdev_get_peripheral_anon(), name,
- OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral_anon(), name,
+ OBJECT(dev), errp);
g_free(name);
}
}
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
{
+ERRP_GUARD();
DeviceClass *dc;
const char *driver, *path;
DeviceState *dev = NULL;
@@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
+if (*errp) {
+goto err_del_dev;
+}
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
--
2.31.1
[PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts
DeviceState.id is a pointer to a string that is stored in the QemuOpts
object DeviceState.opts and freed together with it. We want to create
devices without going through QemuOpts in the future, so make this a
separately allocated string.
Signed-off-by: Kevin Wolf
---
include/hw/qdev-core.h | 2 +-
include/monitor/qdev.h | 2 +-
hw/arm/virt.c | 2 +-
hw/core/qdev.c | 1 +
hw/pci-bridge/pci_expander_bridge.c | 2 +-
hw/ppc/e500.c | 2 +-
softmmu/qdev-monitor.c | 5 +++--
7 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 34c8a7506a..1857d9698e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -176,7 +176,7 @@ struct DeviceState {
Object parent_obj;
/*< public >*/
-const char *id;
+char *id;
char *canonical_path;
bool realized;
bool pending_deleted_event;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index eaa947d73a..389287eb44 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, const char *id);
+void qdev_set_id(DeviceState *dev, char *id);
#endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1d59f0e59f..f933d48d3b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
MemoryRegion *sysmem = get_system_memory();
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
qdev_prop_set_uint32(dev, "mmio_size",
vms->memmap[VIRT_PLATFORM_BUS].size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a..d918b50a1d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
}
qemu_opts_del(dev->opts);
+g_free(dev->id);
}
static void device_class_base_init(ObjectClass *class, void *data)
diff --git a/hw/pci-bridge/pci_expander_bridge.c
b/hw/pci-bridge/pci_expander_bridge.c
index 7112dc3062..10e6e7c2ab 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool
pcie, Error **errp)
} else {
bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
TYPE_PXB_BUS);
bds = qdev_new("pci-bridge");
-bds->id = dev_name;
+bds->id = g_strdup(dev_name);
qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
}
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 95451414dd..960e7efcd3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
/* Platform Bus Device */
if (pmc->has_platform_bus) {
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 034b999401..1207e57a46 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
return bus;
}
-void qdev_set_id(DeviceState *dev, const char *id)
+/* Takes ownership of @id, will be freed when deleting the device */
+void qdev_set_id(DeviceState *dev, char *id)
{
if (id) {
dev->id = id;
@@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, qemu_opts_id(opts));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
--
2.31.1
[PATCH 04/11] qdev: Avoid using string visitor for properties
The only thing the string visitor adds compared to a keyval visitor is
list support. git grep for 'visit_start_list' and 'visit.*List' shows
that devices don't make use of this.
In a world with a QAPIfied command line interface, the keyval visitor is
used to parse the command line. In order to make sure that no devices
start using this feature that would make backwards compatibility harder,
just switch away from object_property_parse(), which internally uses the
string visitor, to a keyval visitor and object_property_set().
Signed-off-by: Kevin Wolf
---
softmmu/qdev-monitor.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0705f00846..034b999401 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -28,6 +28,8 @@
#include "qapi/qmp/dispatch.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
#include "qemu/config-file.h"
#include "qemu/error-report.h"
#include "qemu/help_option.h"
@@ -198,16 +200,28 @@ static int set_property(void *opaque, const char *name,
const char *value,
Error **errp)
{
Object *obj = opaque;
+QString *val;
+Visitor *v;
+int ret;
if (strcmp(name, "driver") == 0)
return 0;
if (strcmp(name, "bus") == 0)
return 0;
-if (!object_property_parse(obj, name, value, errp)) {
-return -1;
+val = qstring_from_str(value);
+v = qobject_input_visitor_new_keyval(QOBJECT(val));
+
+if (!object_property_set(obj, name, v, errp)) {
+ret = -1;
+goto out;
}
-return 0;
+
+ret = 0;
+out:
+visit_free(v);
+qobject_unref(val);
+return ret;
}
static const char *find_typename_by_alias(const char *alias)
--
2.31.1
[PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
Directly call qdev_device_add_from_qdict() for QMP device_add instead of
first going through QemuOpts and converting back to QDict.
Note that this changes the behaviour of device_add, though in ways that
should be considered bug fixes:
QemuOpts ignores differences between data types, so you could
successfully pass a string "123" for an integer property, or a string
"on" for a boolean property (and vice versa). After this change, the
correct data type for the property must be used in the JSON input.
qemu_opts_from_qdict() also silently ignores any options whose value is
a QDict, QList or QNull.
To illustrate, the following QMP command was accepted before and is now
rejected for both reasons:
{ "execute": "device_add",
"arguments": { "driver": "scsi-cd",
"drive": { "completely": "invalid" },
"physical_block_size": "4096" } }
Signed-off-by: Kevin Wolf
---
softmmu/qdev-monitor.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index c09b7430eb..8622ccade6 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
qdev_print_devinfos(true);
}
-void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
+static void monitor_device_add(QDict *qdict, QObject **ret_data,
+ bool from_json, Error **errp)
{
QemuOpts *opts;
DeviceState *dev;
@@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp)
qemu_opts_del(opts);
return;
}
-dev = qdev_device_add(opts, errp);
+qemu_opts_del(opts);
+
+dev = qdev_device_add_from_qdict(qdict, from_json, errp);
/*
* Drain all pending RCU callbacks. This is done because
@@ -838,13 +841,14 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
Error **errp)
*/
drain_call_rcu();
-if (!dev) {
-qemu_opts_del(opts);
-return;
-}
object_unref(OBJECT(dev));
}
+void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
+{
+monitor_device_add(qdict, ret_data, true, errp);
+}
+
static DeviceState *find_device_state(const char *id, Error **errp)
{
Object *obj;
@@ -936,7 +940,7 @@ void hmp_device_add(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
-qmp_device_add((QDict *)qdict, NULL, &err);
+monitor_device_add((QDict *)qdict, NULL, false, &err);
hmp_handle_error(mon, err);
}
--
2.31.1
[PATCH 11/11] Deprecate stable non-JSON -device and -object
We want to switch both from QemuOpts to the keyval parser in the future, which results in some incompatibilities, mainly around list handling. Mark the non-JSON version of both as unstable syntax so that management tools switch to JSON and we can later make the change without breaking things. Signed-off-by: Kevin Wolf --- docs/about/deprecated.rst | 11 +++ 1 file changed, 11 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 3c2be84d80..42f6a478fb 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -160,6 +160,17 @@ Use ``-display sdl`` instead. Use ``-display curses`` instead. +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) +'' + +If you rely on a stable interface for ``-device`` and ``-object`` that doesn't +change incompatibly between QEMU versions (e.g. because you are using the QEMU +command line as a machine interface in scripts rather than interactively), use +JSON syntax for these options instead. + +There is no intention to remove support for non-JSON syntax entirely, but +future versions may change the way to spell some options. + Plugin argument passing through ``arg=`` (since 6.1) -- 2.31.1
[PATCH 10/11] vl: Enable JSON syntax for -device
Like we already do for -object, introduce support for JSON syntax in
-device, which can be kept stable in the long term and guarantees that a
single code path with identical behaviour is used for both QMP and the
command line. Compared to the QemuOpts based code, the parser contains
less surprises and has support for non-scalar options (lists and
structs). Switching management tools to JSON means that we can more
easily change the "human" CLI syntax from QemuOpts to the keyval parser
later.
In the QAPI schema, a feature flag is added to the device-add command to
allow management tools to detect support for this.
Signed-off-by: Kevin Wolf
---
qapi/qdev.json | 15 +
softmmu/vl.c | 58 --
2 files changed, 62 insertions(+), 11 deletions(-)
diff --git a/qapi/qdev.json b/qapi/qdev.json
index b83178220b..cdc8f911b5 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -32,17 +32,23 @@
##
# @device_add:
#
+# Add a device.
+#
# @driver: the name of the new device's driver
#
# @bus: the device's parent bus (device tree path)
#
# @id: the device's ID, must be unique
#
-# Additional arguments depend on the type.
-#
-# Add a device.
+# Features:
+# @json-cli: If present, the "-device" command line option supports JSON
+#syntax with a structure identical to the arguments of this
+#command.
#
# Notes:
+#
+# Additional arguments depend on the type.
+#
# 1. For detailed information about this command, please refer to the
#'docs/qdev-device-use.txt' file.
#
@@ -67,7 +73,8 @@
##
{ 'command': 'device_add',
'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
- 'gen': false } # so we can get the additional arguments
+ 'gen': false, # so we can get the additional arguments
+ 'features': ['json-cli'] }
##
# @device_del:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 55ab70eb97..7596d9da06 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -144,6 +144,12 @@ typedef struct ObjectOption {
QTAILQ_ENTRY(ObjectOption) next;
} ObjectOption;
+typedef struct DeviceOption {
+QDict *opts;
+Location loc;
+QTAILQ_ENTRY(DeviceOption) next;
+} DeviceOption;
+
static const char *cpu_option;
static const char *mem_path;
static const char *incoming;
@@ -151,6 +157,7 @@ static const char *loadvm;
static const char *accelerators;
static QDict *machine_opts_dict;
static QTAILQ_HEAD(, ObjectOption) object_opts =
QTAILQ_HEAD_INITIALIZER(object_opts);
+static QTAILQ_HEAD(, DeviceOption) device_opts =
QTAILQ_HEAD_INITIALIZER(device_opts);
static ram_addr_t maxram_size;
static uint64_t ram_slots;
static int display_remote;
@@ -494,21 +501,39 @@ const char *qemu_get_vm_name(void)
return qemu_name;
}
-static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+static void default_driver_disable(const char *driver)
{
-const char *driver = qemu_opt_get(opts, "driver");
int i;
-if (!driver)
-return 0;
+if (!driver) {
+return;
+}
+
for (i = 0; i < ARRAY_SIZE(default_list); i++) {
if (strcmp(default_list[i].driver, driver) != 0)
continue;
*(default_list[i].flag) = 0;
}
+}
+
+static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+{
+const char *driver = qemu_opt_get(opts, "driver");
+
+default_driver_disable(driver);
return 0;
}
+static void default_driver_check_json(void)
+{
+DeviceOption *opt;
+
+QTAILQ_FOREACH(opt, &device_opts, next) {
+const char *driver = qdict_get_try_str(opt->opts, "driver");
+default_driver_disable(driver);
+}
+}
+
static int parse_name(void *opaque, QemuOpts *opts, Error **errp)
{
const char *proc_name;
@@ -1311,6 +1336,7 @@ static void qemu_disable_default_devices(void)
{
MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
+default_driver_check_json();
qemu_opts_foreach(qemu_find_opts("device"),
default_driver_check, NULL, NULL);
qemu_opts_foreach(qemu_find_opts("global"),
@@ -2637,6 +2663,8 @@ static void qemu_init_board(void)
static void qemu_create_cli_devices(void)
{
+DeviceOption *opt;
+
soundhw_init();
qemu_opts_foreach(qemu_find_opts("fw_cfg"),
@@ -2652,6 +2680,13 @@ static void qemu_create_cli_devices(void)
rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
qemu_opts_foreach(qemu_find_opts("device"),
device_init_func, NULL, &error_fatal);
+QTAILQ_FOREACH(opt, &device_opts, next) {
+QObject *ret_data;
+
+loc_push_restore(&opt->loc);
+qmp_device_add(opt->opts, &ret_data, &error_fatal);
+loc_pop(&opt->loc);
+}
rom_reset_order_override();
}
@@ -3352,9 +3387,18 @@ void qemu_init(int argc, char **argv, char **envp)
add_device_config(DEV_USB, optarg);
break;
case QEMU_OPTION_device:
-if (!
[PATCH 08/11] qdev: Base object creation on QDict rather than QemuOpts
QDicts are both what QMP natively uses and what the keyval parser
produces. Going through QemuOpts isn't useful for either one, so switch
the main device creation function to QDicts. By sharing more code with
the -object/object-add code path, we can even reduce the code size a
bit.
This commit doesn't remove the detour through QemuOpts from any code
path yet, but it allows the following commits to do so.
Signed-off-by: Kevin Wolf
---
include/hw/qdev-core.h | 8 ++---
hw/core/qdev.c | 5 ++--
hw/net/virtio-net.c| 4 +--
hw/vfio/pci.c | 4 +--
softmmu/qdev-monitor.c | 67 +++---
5 files changed, 41 insertions(+), 47 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1857d9698e..5b3d4704a5 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -180,7 +180,7 @@ struct DeviceState {
char *canonical_path;
bool realized;
bool pending_deleted_event;
-QemuOpts *opts;
+QDict *opts;
int hotplugged;
bool allow_unplug_during_migration;
BusState *parent_bus;
@@ -202,7 +202,7 @@ struct DeviceListener {
* hide a failover device depending for example on the device
* opts.
*/
-bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts);
+bool (*hide_device)(DeviceListener *listener, const QDict *device_opts);
QTAILQ_ENTRY(DeviceListener) link;
};
@@ -831,13 +831,13 @@ void device_listener_unregister(DeviceListener *listener);
/**
* @qdev_should_hide_device:
- * @opts: QemuOpts as passed on cmdline.
+ * @opts: options QDict
*
* Check if a device should be added.
* When a device is added via qdev_device_add() this will be called,
* and return if the device should be added now or not.
*/
-bool qdev_should_hide_device(QemuOpts *opts);
+bool qdev_should_hide_device(const QDict *opts);
typedef enum MachineInitPhase {
/* current_machine is NULL. */
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d918b50a1d..5b889866c5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -28,6 +28,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qapi/qapi-events-qdev.h"
+#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
#include "qapi/visitor.h"
#include "qemu/error-report.h"
@@ -211,7 +212,7 @@ void device_listener_unregister(DeviceListener *listener)
QTAILQ_REMOVE(&device_listeners, listener, link);
}
-bool qdev_should_hide_device(QemuOpts *opts)
+bool qdev_should_hide_device(const QDict *opts)
{
DeviceListener *listener;
@@ -955,7 +956,7 @@ static void device_finalize(Object *obj)
dev->canonical_path = NULL;
}
-qemu_opts_del(dev->opts);
+qobject_unref(dev->opts);
g_free(dev->id);
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f205331dcf..5684c2b2b7 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3304,7 +3304,7 @@ static void virtio_net_migration_state_notifier(Notifier
*notifier, void *data)
}
static bool failover_hide_primary_device(DeviceListener *listener,
- QemuOpts *device_opts)
+ const QDict *device_opts)
{
VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
const char *standby_id;
@@ -3312,7 +3312,7 @@ static bool failover_hide_primary_device(DeviceListener
*listener,
if (!device_opts) {
return false;
}
-standby_id = qemu_opt_get(device_opts, "failover_pair_id");
+standby_id = qdict_get_try_str(device_opts, "failover_pair_id");
if (g_strcmp0(standby_id, n->netclient_name) != 0) {
return false;
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4feaa1cb68..5cdf1d4298 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -29,10 +29,10 @@
#include "hw/qdev-properties.h"
#include "hw/qdev-properties-system.h"
#include "migration/vmstate.h"
+#include "qapi/qmp/qdict.h"
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
#include "qemu/module.h"
-#include "qemu/option.h"
#include "qemu/range.h"
#include "qemu/units.h"
#include "sysemu/kvm.h"
@@ -941,7 +941,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
}
if (vfio_opt_rom_in_denylist(vdev)) {
-if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
+if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
warn_report("Device at %s is known to cause system instability"
" issues during option rom execution",
vdev->vbasedev.name);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index c2af906df0..c09b7430eb 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -196,34 +196,6 @@ static void qdev_print_devinfos(bool show_no_user)
g_slist_free(list);
}
-static int set_property(void *opaque, const char *name, const char *value,
-Error **errp)
-{
-Object *obj = opaqu
[PATCH 07/11] qemu-option: Allow deleting opts during qemu_opts_foreach()
Use QTAILQ_FOREACH_SAFE() so that the current QemuOpts can be deleted
while iterating through the whole list.
Signed-off-by: Kevin Wolf
---
util/qemu-option.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 61cb4a97bd..eedd08929b 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1126,11 +1126,11 @@ int qemu_opts_foreach(QemuOptsList *list,
qemu_opts_loopfunc func,
void *opaque, Error **errp)
{
Location loc;
-QemuOpts *opts;
+QemuOpts *opts, *next;
int rc = 0;
loc_push_none(&loc);
-QTAILQ_FOREACH(opts, &list->head, next) {
+QTAILQ_FOREACH_SAFE(opts, &list->head, next, next) {
loc_restore(&opts->loc);
rc = func(opaque, opts, errp);
if (rc) {
--
2.31.1
Re: [PATCH v2 1/2] virsh: domain: fix mistake in cmdMigrateSetMaxDowntime()
On 9/24/21 1:30 AM, Kristina Hanicova wrote: > Function returned false when everything ended successfully, there > was a missing check for a return value. This patch fixes that. > > Signed-off-by: Kristina Hanicova > --- > tools/virsh-domain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index f876f30cc5..2730acfba5 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -11018,7 +11018,7 @@ cmdMigrateSetMaxDowntime(vshControl *ctl, const > vshCmd *cmd) > goto done; > } > > -if (virDomainMigrateSetMaxDowntime(dom, downtime, 0)) > +if (virDomainMigrateSetMaxDowntime(dom, downtime, 0) < 0) > goto done; > > ret = true; > Technically, there wasn't any bug here. virDomainMigrateSetMaxDowntime() returns 0 on success and 0 won't evaluate to true thus 'goto done' is skipped over and ret is set to true. And if anything else is returned (currently only -1 is possible) then i's evaluated to true and control jump to the done label. BUT, your fix is correct because in libvirt only negative values are treated as errors. Zero and positive values are treated as success. I'll reword the commit message a bit. Sorry for not realizing this earlier. Michal
[PATCH 02/11] iotests/245: Fix type for iothread property
iothread is a string property, so None (= JSON null) is not a valid
value for it. Pass the empty string instead to get the default iothread.
Signed-off-by: Kevin Wolf
---
tests/qemu-iotests/245 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bf8261eec0..9b12b42eed 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1189,10 +1189,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.run_test_iothreads('iothread0', 'iothread0')
def test_iothreads_switch_backing(self):
-self.run_test_iothreads('iothread0', None)
+self.run_test_iothreads('iothread0', '')
def test_iothreads_switch_overlay(self):
-self.run_test_iothreads(None, 'iothread0')
+self.run_test_iothreads('', 'iothread0')
if __name__ == '__main__':
iotests.activate_logging()
--
2.31.1
[PATCH 01/11] qom: Reduce use of error_propagate()
ERRP_GUARD() makes debugging easier by making sure that &error_abort
still fails at the real origin of the error instead of
error_propagate().
Signed-off-by: Kevin Wolf
---
qom/object.c| 7 +++
qom/object_interfaces.c | 17 ++---
2 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index e86cb05b84..6be710bc40 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1389,7 +1389,7 @@ bool object_property_get(Object *obj, const char *name,
Visitor *v,
bool object_property_set(Object *obj, const char *name, Visitor *v,
Error **errp)
{
-Error *err = NULL;
+ERRP_GUARD();
ObjectProperty *prop = object_property_find_err(obj, name, errp);
if (prop == NULL) {
@@ -1400,9 +1400,8 @@ bool object_property_set(Object *obj, const char *name,
Visitor *v,
error_setg(errp, QERR_PERMISSION_DENIED);
return false;
}
-prop->set(obj, v, name, prop->opaque, &err);
-error_propagate(errp, err);
-return !err;
+prop->set(obj, v, name, prop->opaque, errp);
+return !*errp;
}
bool object_property_set_str(Object *obj, const char *name,
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ad9b56b59a..80691e88cd 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -45,26 +45,21 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
Visitor *v, Error **errp)
{
+ERRP_GUARD();
const QDictEntry *e;
-Error *local_err = NULL;
-if (!visit_start_struct(v, NULL, NULL, 0, &local_err)) {
-goto out;
+if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
+return;
}
for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-if (!object_property_set(obj, e->key, v, &local_err)) {
+if (!object_property_set(obj, e->key, v, errp)) {
break;
}
}
-if (!local_err) {
-visit_check_struct(v, &local_err);
+if (!*errp) {
+visit_check_struct(v, errp);
}
visit_end_struct(v, NULL);
-
-out:
-if (local_err) {
-error_propagate(errp, local_err);
-}
}
void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
--
2.31.1
Re: [PATCH v2 2/2] virsh: domain: remove unnecessary variable and label in cmdMigrateSetMaxDowntime()
On 9/24/21 1:30 AM, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova
> ---
> tools/virsh-domain.c | 14 --
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 2730acfba5..cfa0016296 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -11006,25 +11006,19 @@ cmdMigrateSetMaxDowntime(vshControl *ctl, const
> vshCmd *cmd)
> {
> g_autoptr(virshDomain) dom = NULL;
> unsigned long long downtime = 0;
> -bool ret = false;
>
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> return false;
>
> if (vshCommandOptULongLong(ctl, cmd, "downtime", &downtime) < 0)
> -goto done;
> +return false;
> +
> if (downtime < 1) {
> vshError(ctl, "%s", _("migrate: Invalid downtime"));
> -goto done;
> +return false;
> }
>
> -if (virDomainMigrateSetMaxDowntime(dom, downtime, 0) < 0)
> -goto done;
> -
> -ret = true;
> -
> - done:
> -return ret;
> +return virDomainMigrateSetMaxDowntime(dom, downtime, 0) == 0;
> }
>
>
>
Honestly, I'm not big fan of return func() == 0 type of statements. But
I think I can live with this.
Michal
Re: [PATCH v2 3/4] virsh: remove variable 'ret' and 'inactive'
On Fri, Sep 24, 2021 at 10:02:07 +0200, Michal Prívozník wrote:
> On 9/24/21 1:49 AM, Kristina Hanicova wrote:
> > Signed-off-by: Kristina Hanicova
> > ---
> > tools/virsh-interface.c | 15 ++-
> > tools/virsh-network.c | 17 ++---
> > 2 files changed, 12 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
> > index 4bcc59b580..f402119b68 100644
> > --- a/tools/virsh-interface.c
> > +++ b/tools/virsh-interface.c
> > @@ -485,26 +485,23 @@ static bool
> > cmdInterfaceDumpXML(vshControl *ctl, const vshCmd *cmd)
> > {
> > virInterfacePtr iface;
>
> 1: this ^^^
>
> >
> > +vshPrint(ctl, "%s", dump);
> > virInterfaceFree(iface);
> > -return ret;
> > +return true;
>
> I wonder whether we can declare an g_autoptr() cleanup for these public
> APIs. We can't do that in public headers BUT we could do it in something
> private, that's widely available (internal.h perhaps?). That way we
> could turn [1] into g_autoptr() and drop both these free calls.
See 'virshDomain' and the associated cleanup registration in
virsh-util.h
Also
https://listman.redhat.com/archives/libvir-list/2021-September/msg00751.html
> Obviously, that's orthogonal to what you are doing here and as such must
> go into a standalone patchset. Just an idea I had.
>
> Michal
>
Re: [PATCH v2 0/4] virsh: refactor unnecessary variables and else branch
On 9/24/21 1:49 AM, Kristina Hanicova wrote: > This is partially v2 of: > https://listman.redhat.com/archives/libvir-list/2021-September/msg00731.html > > Diff to v1: > * split changes into smaller patches > > Kristina Hanicova (4): > virsh: remove variable 'ret' and use early return if possible > virsh: remove variable 'ret' in cmdVersion() > virsh: remove variable 'ret' and 'inactive' > virsh: util: remove 'else' branch after return > > tools/virsh-host.c | 25 +--- > tools/virsh-interface.c | 65 ++--- > tools/virsh-network.c | 47 + > tools/virsh-nodedev.c | 32 > tools/virsh-nwfilter.c | 15 +- > tools/virsh-util.c | 3 +- > 6 files changed, 76 insertions(+), 111 deletions(-) > Reviewed-by: Michal Privoznik Michal
Re: [PATCH v2 3/4] virsh: remove variable 'ret' and 'inactive'
On 9/24/21 1:49 AM, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova
> ---
> tools/virsh-interface.c | 15 ++-
> tools/virsh-network.c | 17 ++---
> 2 files changed, 12 insertions(+), 20 deletions(-)
>
> diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
> index 4bcc59b580..f402119b68 100644
> --- a/tools/virsh-interface.c
> +++ b/tools/virsh-interface.c
> @@ -485,26 +485,23 @@ static bool
> cmdInterfaceDumpXML(vshControl *ctl, const vshCmd *cmd)
> {
> virInterfacePtr iface;
1: this ^^^
> -bool ret = true;
> g_autofree char *dump = NULL;
> unsigned int flags = 0;
> -bool inactive = vshCommandOptBool(cmd, "inactive");
>
> -if (inactive)
> +if (vshCommandOptBool(cmd, "inactive"))
> flags |= VIR_INTERFACE_XML_INACTIVE;
>
> if (!(iface = virshCommandOptInterface(ctl, cmd, NULL)))
> return false;
>
> -dump = virInterfaceGetXMLDesc(iface, flags);
> -if (dump != NULL) {
> -vshPrint(ctl, "%s", dump);
> -} else {
> -ret = false;
> +if (!(dump = virInterfaceGetXMLDesc(iface, flags))) {
> +virInterfaceFree(iface);
> +return false;
> }
>
> +vshPrint(ctl, "%s", dump);
> virInterfaceFree(iface);
> -return ret;
> +return true;
I wonder whether we can declare an g_autoptr() cleanup for these public
APIs. We can't do that in public headers BUT we could do it in something
private, that's widely available (internal.h perhaps?). That way we
could turn [1] into g_autoptr() and drop both these free calls.
Obviously, that's orthogonal to what you are doing here and as such must
go into a standalone patchset. Just an idea I had.
Michal
Re: [PATCH v2 1/6] virsh: domain: refactor cmdSchedinfo()
On 9/24/21 1:25 AM, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova
> ---
> tools/virsh-domain.c | 99 +---
> 1 file changed, 47 insertions(+), 52 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index f876f30cc5..b64df640ba 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -5157,7 +5157,6 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
> int nparams = 0;
> int nupdates = 0;
> size_t i;
> -int ret;
> bool ret_val = false;
> unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> bool current = vshCommandOptBool(cmd, "current");
> @@ -5176,73 +5175,69 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
> return false;
>
> /* Print SchedulerType */
> -schedulertype = virDomainGetSchedulerType(dom, &nparams);
> -if (schedulertype != NULL) {
> -vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype);
> -VIR_FREE(schedulertype);
> -} else {
> +if (!(schedulertype = virDomainGetSchedulerType(dom, &nparams))) {
> vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), _("Unknown"));
> goto cleanup;
> }
>
> -if (nparams) {
> -params = g_new0(virTypedParameter, nparams);
> +vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), schedulertype);
> +VIR_FREE(schedulertype);
I think instead of adding this VIR_FREE() you can just mark the variable
as g_autofree. It's a negligible change compared how much lines are
being changed.
>
> -memset(params, 0, sizeof(*params) * nparams);
> -if (flags || current) {
> -/* We cannot query both live and config at once, so settle
> - on current in that case. If we are setting, then the
> - two values should match when we re-query; otherwise, we
> - report the error later. */
> -ret = virDomainGetSchedulerParametersFlags(dom, params, &nparams,
> - ((live && config) ? 0
> -: flags));
> -} else {
> -ret = virDomainGetSchedulerParameters(dom, params, &nparams);
> -}
> -if (ret == -1)
> -goto cleanup;
> +if (!nparams)
> +goto cleanup;
>
> -/* See if any params are being set */
> -if ((nupdates = cmdSchedInfoUpdate(ctl, cmd, params, nparams,
> - &updates)) < 0)
> +params = g_new0(virTypedParameter, nparams);
> +memset(params, 0, sizeof(*params) * nparams);
> +
> +if (flags || current) {
> +/* We cannot query both live and config at once, so settle
> + on current in that case. If we are setting, then the
> + two values should match when we re-query; otherwise, we
> + report the error later. */
> +if (virDomainGetSchedulerParametersFlags(dom, params, &nparams,
> + ((live && config) ? 0 :
> flags)) == -1)
> +goto cleanup;
> +} else {
> +if (virDomainGetSchedulerParameters(dom, params, &nparams) == -1)
> goto cleanup;
> +}
>
> -/* Update parameters & refresh data */
> -if (nupdates > 0) {
> -if (flags || current)
> -ret = virDomainSetSchedulerParametersFlags(dom, updates,
> - nupdates, flags);
> -else
> -ret = virDomainSetSchedulerParameters(dom, updates,
> nupdates);
> +/* See if any params are being set */
> +if ((nupdates = cmdSchedInfoUpdate(ctl, cmd, params, nparams,
> + &updates)) < 0)
> +goto cleanup;
>
> -if (ret == -1)
> +/* Update parameters & refresh data */
> +if (nupdates > 0) {
> +if (flags || current) {
> +if (virDomainSetSchedulerParametersFlags(dom, updates,
> + nupdates, flags) == -1)
> goto cleanup;
>
> -if (flags || current)
> -ret = virDomainGetSchedulerParametersFlags(dom, params,
> - &nparams,
> - ((live && config)
> ? 0
> -: flags));
> -else
> -ret = virDomainGetSchedulerParameters(dom, params, &nparams);
> -if (ret == -1)
> +if (virDomainGetSchedulerParametersFlags(dom, params, &nparams,
> + ((live && config) ? 0 :
> flags)) == -1)
> goto cleanup;
> } else {
> -/* When not doing --set, --live and --config do not mix. */
> -if (live && config) {
> -
Re: [PATCH v2 0/6] virsh: refactor some bigger functions
On 9/24/21 1:25 AM, Kristina Hanicova wrote: > This is v2 of patch: > https://listman.redhat.com/archives/libvir-list/2021-September/msg00730.html > > Diff to v1: > * split the previous patch into more smaller ones to make the code > review easier > * small code quality alternations I did not notice were possible before > > This series refactors a few bigger functions mainly trying to remove too > deep indentation and make them more readable and more consistent with > the rest of the files. > > Kristina Hanicova (6): > virsh: domain: refactor cmdSchedinfo() > virsh: domain: refactor virshCPUCountCollect() > virsh: domain: refactor cmdLxcEnterNamespace() > virsh: host: refactor cmdFreecell() > virsh: host: refactor cmdNodeCpuStats() > virsh: volume: refactor cmdVolInfo() > > tools/virsh-domain.c | 210 --- > tools/virsh-host.c | 139 ++-- > tools/virsh-volume.c | 52 +-- > 3 files changed, 189 insertions(+), 212 deletions(-) > Reviewed-by: Michal Privoznik I've done the change suggested in 1/6 and pushed. Michal
