Re: [libvirt] [PATCH 12/12] qemu: driver: allow remote destinations for block copy
On Thu, Aug 08, 2019 at 06:00:42PM +0200, Peter Krempa wrote: Now that we support blockdev for qemuDomainBlockCopy we can allow copying to remote destinations as well. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 47 -- 1 file changed, 32 insertions(+), 15 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/12] qemu: Add blockdev support for the block copy job
On Thu, Aug 08, 2019 at 06:00:41PM +0200, Peter Krempa wrote: Implement job handling for the block copy job (drive/blockdev-mirror) when using -blockdev. In contrast to the previously implemented blockjobs the block copy job introduces new images to the running qemu instance, thus requires a bit more handling. When copying to new images the code now makes use of blockdev-create to format the images explicitly rather than depending on automagic qemu behaviour. Signed-off-by: Peter Krempa --- src/qemu/qemu_blockjob.c | 87 + src/qemu/qemu_blockjob.h | 16 +++ src/qemu/qemu_domain.c| 13 +++ src/qemu/qemu_driver.c| 97 --- .../blockjob-blockdev-in.xml | 14 +++ 5 files changed, 216 insertions(+), 11 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/12] qemu: Introduce code for blockdev-create
On Thu, Aug 08, 2019 at 06:00:40PM +0200, Peter Krempa wrote:
QEMU finally exposes an interface which allows us to instruct it to
format or create arbitrary images. This is required for blockdev
integration of block copy and snapshots as we need to pre-format images
prior to use with blockdev-add.
This path introduces job handling and also helpers for formatting and
attaching a whole image described by a virStorageSource.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 250 ++
src/qemu/qemu_block.h | 14 +
src/qemu/qemu_blockjob.c | 88 +-
src/qemu/qemu_blockjob.h | 17 ++
src/qemu/qemu_domain.c| 34 ++-
src/qemu/qemu_driver.c| 1 +
.../blockjob-blockdev-in.xml | 45
7 files changed, 446 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 47661fb8bd..cb9a085e5d 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
[...]
+static int
+qemuBlockStorageSourceCreateStorage(virDomainObjPtr vm,
+virStorageSourcePtr src,
+virStorageSourcePtr chain,
+qemuDomainAsyncJob asyncJob)
+{
+int actualType = virStorageSourceGetActualType(src);
+VIR_AUTOPTR(virJSONValue) createstorageprops = NULL;
+int ret;
+
+/* we need to do stuff only for remote storage and local raw files */
Rather than rewording the following condition, it would be better to say
why we do not need to do it.
+if (actualType != VIR_STORAGE_TYPE_NETWORK &&
+!(actualType == VIR_STORAGE_TYPE_FILE && src->format ==
VIR_STORAGE_FILE_RAW))
+return 0;
+
+if (qemuBlockStorageSourceCreateGetStorageProps(src, )
< 0)
+return -1;
+
+if (!createstorageprops) {
+/* we can always try opening it to see whether it was existing */
+return 0;
+}
+
+ret = qemuBlockStorageSourceCreateGeneric(vm, createstorageprops, src,
chain,
+ true, asyncJob);
+createstorageprops = NULL;
+
+return ret;
+}
+
+
+static int
+qemuBlockStorageSourceCreateFormat(virDomainObjPtr vm,
+ virStorageSourcePtr src,
+ virStorageSourcePtr backingStore,
+ virStorageSourcePtr chain,
+ qemuDomainAsyncJob asyncJob)
+{
+VIR_AUTOPTR(virJSONValue) createformatprops = NULL;
+int ret;
+
+if (src->format == VIR_STORAGE_FILE_RAW)
+return 0;
+
+if (qemuBlockStorageSourceCreateGetFormatProps(src, backingStore,
+ ) < 0)
+return -1;
+
+if (!createformatprops) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("can't create storage format '%s'"),
+ virStorageFileFormatTypeToString(src->format));
+return -1;
+}
+
+ret = qemuBlockStorageSourceCreateGeneric(vm, createformatprops, src,
chain,
+ false, asyncJob);
+createformatprops = NULL;
+
+return ret;
+}
+
+
+/**
+ * qemuBlockStorageSourceCreate:
+ * @vm: domain object
+ * @src: storage source definition to create
+ * @backingStore: backingStore of the new image (used only in image metadata)
+ * @chain: backing chain to unplug in case of a long-running job failure
+ * @data: qemuBlockStorageSourceAttachData for @src so that it can be attached
+ * @asyncJob: qemu asynchronous job type
+ *
+ * Creates and formats a storage volume according to @src and attaches it to
@vm.
+ * @data must provide attachment data as if @src was existing. @src is attached
+ * after successful return of this function. If libvirtd is restarted during
+ * the create job @chain is unplugged, otherwise it's left for the caller.
+ * If @backingStore is provided, the new image will refer to it as it's backing
*its
+ * store.
+ */
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Exposing feature deprecation to machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters)
John Snow writes:
> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> To get rid of implicit filters related workarounds in future let's
>> deprecate them now.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
[...]
>> diff --git a/blockdev.c b/blockdev.c
>> index 36e9368e01..b3cfaccce1 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
>> *job_id, const char *device,
>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>> int job_flags = JOB_DEFAULT;
>>
>> +if (!has_filter_node_name) {
>> +warn_report("Omitting filter-node-name parameter is deprecated, it "
>> +"will be required in future");
>> +}
>> +
>> if (!has_speed) {
>> speed = 0;
>> }
>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
>> *job_id,
>> Error *local_err = NULL;
>> int ret;
>>
>> +if (!has_filter_node_name) {
>> +warn_report("Omitting filter-node-name parameter is deprecated, it "
>> +"will be required in future");
>> +}
>> +
>> bs = qmp_get_root_bs(device, errp);
>> if (!bs) {
>> return;
>>
>
> This might be OK to do right away, though.
>
> I asked Markus this not too long ago; do we want to amend the QAPI
> schema specification to allow commands to return with "Warning" strings,
> or "Deprecated" stings to allow in-band deprecation notices for cases
> like these?
>
> example:
>
> { "return": {},
> "deprecated": True,
> "warning": "Omitting filter-node-name parameter is deprecated, it will
> be required in the future"
> }
>
> There's no "error" key, so this should be recognized as success by
> compatible clients, but they'll definitely see the extra information.
This is a compatible evolution of the QMP protocol.
> Part of my motivation is to facilitate a more aggressive deprecation of
> legacy features by ensuring that we are able to rigorously notify users
> through any means that they need to adjust their scripts.
Yes, we should help libvirt etc. with detecting use of deprecated
features. We discussed this at the KVM Forum 2018 BoF on deprecating
stuff. Minutes:
Message-ID: <87mur0ls8o@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
Last item is relevant here.
Adding deprecation information to QMP's success response belongs to "We
can also pass the buck to the next layer up", next to "emit a QMP
event".
Let's compare the two, i.e. "deprecation info in success response"
vs. "deprecation event".
1. Possible triggers
Anything we put in the success response should only ever apply to the
(successful) command. So this one's limited to QMP commands.
A QMP event is not limited to QMP commands. For instance, it could be
emitted for deprecated CLI features (long after the fact, in addition to
human-readable warnings on stderr), or when we detect use of a
deprecated feature only after we sent the success response, say in a
job. Neither use case is particularly convincing. Reporting use of
deprecated CLI in QMP feels like a work-around for the CLI's
machine-unfriendliness. Job-like commands should really check their
arguments upfront.
2. Connection to trigger
Connecting responses to commands is the QMP protocol's responsibility.
Transmitting deprecation information in the response trivially ties it
to the offending command.
The QMP protocol doesn't tie events to anything. Thus, a deprecation
event needs an event-specific tie to its trigger.
The obvious way to tie it to a command mirrors how the QMP protocol ties
responses to commands: by command ID. The event either has to be sent
just to the offending monitor (currently, all events are broadcast to
all monitors), or include a suitable monitor ID.
For non-command triggers, we'd have to invent some other tie.
3. Interface complexity
Tying the event to some arbitrary trigger adds complexity.
Do we need non-command triggers, and badly enough to justify the
additional complexity?
4. Implementation complexity
Emitting an event could be as simple as
qapi_event_send_deprecated(qmp_command_id(),
"Omitting 'filter-node-name'");
where qmp_command_id() returns the ID of the currently executing
command. Making qmp_command_id() work is up to the QMP core. Simple
enough as long as each QMP command runs to completion before its monitor
starts the next one.
The event is "fire and forget". There is no warning object propagated
up the call chain into the QMP core like errors objects are.
"Fire and forget" is ideal for letting arbitrary code decide "this is
deprecated".
Note the QAPI schema remains untouched.
Unlike an event, which can be emitted anywhere, the success response
gets built in the QMP core. To have the core add deprecation info to
it, we need to get the info to the core.
If deprecation info originates in
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
On Thu, Aug 15, 2019 at 12:49:28 +0200, Kevin Wolf wrote:
> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
[...]
> > example:
> >
> > { "return": {},
> > "deprecated": True,
> > "warning": "Omitting filter-node-name parameter is deprecated, it will
> > be required in the future"
> > }
> >
> > There's no "error" key, so this should be recognized as success by
> > compatible clients, but they'll definitely see the extra information.
> >
> > Part of my motivation is to facilitate a more aggressive deprecation of
> > legacy features by ensuring that we are able to rigorously notify users
> > through any means that they need to adjust their scripts.
>
> Who would read this, though? In the best case it ends up deep in a
> libvirt log that nobody will look at because there was no error. In the
> more common case, the debug level is configured so that QMP traffic
> isn't even logged.
The best we could do here is to log a warning. Thankfully we have one
central function which always checks the returned JSON from qemu so we
could do that universally.
The would end up in the system log and alternatively also in the VM
log file. I agree with Kevin that the possibility of it being noticed
is rather small.
From my experience users report non-fatal messages mostly only if it is
spamming the system log. One of instances are very unlikely to be
noticed.
In my experience it's better to notify us in libvirt of such change and
we will try our best to fix it.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: Make anchors in API html files clickable/linkable
On Thu, Aug 15, 2019 at 10:21:59AM +0200, Peter Krempa wrote: Use 'id' instead of 'name' for anchors which adds the hidden clickable headerlink helper so it's way simpler to link to a specific part of the docs. was deprecated in favor of anyway. Signed-off-by: Peter Krempa --- docs/newapi.xsl | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) Reviewed-by: Ján Tomko There are also three more occurrences of inside a heading: $ git grep -n 'h.*a name' docs/newapi.xsl docs/newapi.xsl:786: Macros docs/newapi.xsl:791:Types docs/newapi.xsl:795:Functions Feel free to include the changes here. Or not. Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Peter Krempa writes:
> On Thu, Aug 15, 2019 at 12:49:28 +0200, Kevin Wolf wrote:
>> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>
> [...]
>
>> > example:
>> >
>> > { "return": {},
>> > "deprecated": True,
>> > "warning": "Omitting filter-node-name parameter is deprecated, it will
>> > be required in the future"
>> > }
>> >
>> > There's no "error" key, so this should be recognized as success by
>> > compatible clients, but they'll definitely see the extra information.
>> >
>> > Part of my motivation is to facilitate a more aggressive deprecation of
>> > legacy features by ensuring that we are able to rigorously notify users
>> > through any means that they need to adjust their scripts.
>>
>> Who would read this, though? In the best case it ends up deep in a
>> libvirt log that nobody will look at because there was no error. In the
>> more common case, the debug level is configured so that QMP traffic
>> isn't even logged.
>
> The best we could do here is to log a warning. Thankfully we have one
> central function which always checks the returned JSON from qemu so we
> could do that universally.
>
> The would end up in the system log and alternatively also in the VM
> log file. I agree with Kevin that the possibility of it being noticed
> is rather small.
>
> From my experience users report non-fatal messages mostly only if it is
> spamming the system log. One of instances are very unlikely to be
> noticed.
>
> In my experience it's better to notify us in libvirt of such change and
> we will try our best to fix it.
How to best alert the layers above QEMU was one of the topic of the KVM
Forum 2018 BoF on deprecating stuff. Minutes:
Message-ID: <87mur0ls8o@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
Relevant part:
* We need to communicate "you're using something that is deprecated".
How? Right now, we print a deprecation message. Okay when humans use
QEMU directly in a shell. However, when QEMU sits at the bottom of a
software stack, the message will likely end up in a log file that is
effectively write-only.
- The one way to get people read log files is crashing their
application. A command line option --future could make QEMU crash
right after printing a deprecation message. This could help with
finding use of deprecated features in a testing environment.
- A less destructive way to grab people's attention is to make things
run really, really slow: have QEMU go to sleep for a while after
printing a deprecation message.
- We can also pass the buck to the next layer up: emit a QMP event.
Sadly, by the time the next layer connects to QMP, plenty of stuff
already happened. We'd have to buffer deprecation events somehow.
What would libvirt do with such an event? Log it, taint the domain,
emit a (libvirt) event to pass it on to the next layer up.
- A completely different idea is to have a configuratin linter. To
support doing this at the libvirt level, QEMU could expose "is
deprecated" in interface introspection. Feels feasible for QMP,
where we already have sufficiently expressive introspection. For
CLI, we'd first have to provide that (but we want that anyway).
- We might also want to dispay deprecation messages in QEMU's GUI
somehow, or on serial consoles.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virpci:fix Secondary Bus Reset bug
From: hexin The parent bridge configuration of the current device should be read and reset, instead of reading the current device configuration. Signed-off-by: He Xin Signed-off-by: Liu Qi Signed-off-by: Zhang Yu --- src/util/virpci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 61a6b359e5..483de2cb16 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -821,7 +821,7 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev, /* Read the control register, set the reset flag, wait 200ms, * unset the reset flag and wait 200ms. */ -ctl = virPCIDeviceRead16(dev, cfgfd, PCI_BRIDGE_CONTROL); +ctl = virPCIDeviceRead16(dev, parentfd, PCI_BRIDGE_CONTROL); virPCIDeviceWrite16(parent, parentfd, PCI_BRIDGE_CONTROL, ctl | PCI_BRIDGE_CTL_RESET); -- 2.22.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 1/2] qapi: deprecate drive-mirror and drive-backup
On Wed, Aug 14, 2019 at 15:22:15 -0400, John Snow wrote:
>
>
> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> > It's hard and not necessary to maintain outdated versions of these
> > commands.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > ---
> > qemu-deprecated.texi | 4
> > qapi/block-core.json | 4
> > qapi/transaction.json | 2 +-
> > blockdev.c| 10 ++
> > 4 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index fff07bb2a3..2753fafd0b 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -179,6 +179,10 @@ and accurate ``query-qmp-schema'' command.
> > Character devices creating sockets in client mode should not specify
> > the 'wait' field, which is only applicable to sockets in server mode
> >
> > +@subsection drive-mirror, drive-backup and drive-backup transaction action
> > (since 4.2)
> > +
> > +Use blockdev-mirror and blockdev-backup instead.
> > +
> > @section Human Monitor Protocol (HMP) commands
> >
> > @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
> > (since 3.1)
[...]
> > @@ -3831,6 +3838,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
> > const char *format = arg->format;
> > int ret;
> >
> > +warn_report("drive-mirror command is deprecated and will disappear in "
> > +"future. Use blockdev-mirror instead");
> > +
> > bs = qmp_get_root_bs(arg->device, errp);
> > if (!bs) {
> > return;
> >
>
> Hm!
>
> I wonder if this is ever-so-slightly too soon for our friends over at
> the libvirt project.
>
> I don't think they have fully moved away from the non-blockdev
> interfaces *just yet*, and I might encourage seeing the first full
> libvirt release that does support and use it before we start the
> deprecation clock.
>
> (Jst in case.)
>
> That's just me being very, very cautious though.
>
> Peter Krempa, how do you feel about this?
Thanks for the heads up!
Currently libvirt does not use 'drive-backup' at all so that one can be
deprecated immediately.
In case of 'drive-mirror' the situation is a bit more complex:
Libvirt uses 'drive-mirror' currently in the following places
1) virDomainBlockCopy API
With blockdev integration enabled this will go away. Pathces are being
reviewed:
https://www.redhat.com/archives/libvir-list/2019-August/msg00295.html
2) VM migration with non-shared storage
Currently uses 'drive-mirror' in most cases but there is pre-existing
integration for blockdev-mirror for nbd+tls. I need to make sure that
the blockdev version will be used unconditionally once the integration
is enabled. This is a TODO.
There is also one gotcha. In case when an 'sd' card device is used for
the VM, libvirt disables all of blockdev, because SD cards can't be
expressed with blockdev. There's too many code paths which would need
checking to be worth it. To be fair, I'm not even sure when a sd card
can be emulated by qemu as all of my basic tests failed and I did not
care more.
For libvirt to enable blockdev there's one more part missing and that's
snapshot integration. I'm currently testing patches to integrate it with
external snapshots, which should be posted soon.
I also found a bug in qemu, which prevents creation of internal
snapshots when -blockdev is used:
When savevm HMP command is used (via QMP->HMP bridge) qemu invokes
save_snapshot(), which calls bdrv_all_can_snapshot(). That function uses
bdrv_next() to iterate all nodes which correspond to a block backend
first, but then also iterates any other node which is monitor-owned.
Since with blockdev all nodes including the ones for the 'file' protocol
are monitor-owned, and 'file' does not support snapshots that check
fails. A simple hack of skipping the second part in bdrv_next() allows
to do a snapshot actually. Kevin told me that the idea is that also
non-attached nodes should be considered for internal snapshod which is
okay in my opinion, but given how the snapshot works for the files
attached to backeds (and also in pre-blockdev use) only the top level of
a chain should ever be considered for snapshot.
So the summary is, that I'm pretty hopeful that we should be able to get
rid of all reasonable uses of drive-mirror very soon after I finish
snapshot integration. The only question is how much
we care about SD card users being able to do a drive-mirror in the
future.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-block] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
On Wed, 2019-08-14 at 15:27 -0400, John Snow wrote:
>
> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> > To get rid of implicit filters related workarounds in future let's
> > deprecate them now.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > ---
> > qemu-deprecated.texi | 7 +++
> > qapi/block-core.json | 6 --
> > include/block/block_int.h | 10 +-
> > blockdev.c| 10 ++
> > 4 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index 2753fafd0b..8222440148 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets
> > in server mode
> >
> > Use blockdev-mirror and blockdev-backup instead.
> >
> > +@subsection implicit filters (since 4.2)
> > +
> > +Mirror and commit jobs inserts filters, which becomes implicit if user
> > +omitted filter-node-name parameter. So omitting it is deprecated, set it
> > +always. Note, that drive-mirror don't have this parameter, so it will
> > +create implicit filter anyway, but drive-mirror is deprecated itself too.
> > +
> > @section Human Monitor Protocol (HMP) commands
> >
> > @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
> > (since 3.1)
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 4e35526634..0505ac9d8b 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1596,7 +1596,8 @@
> > # @filter-node-name: the node name that should be assigned to the
> > #filter driver that the commit job inserts into the
> > graph
> > #above @top. If this option is not given, a node name
> > is
> > -#autogenerated. (Since: 2.9)
> > +#autogenerated. Omitting this option is deprecated, it
> > will
> > +#be required in future. (Since: 2.9)
> > #
> > # @auto-finalize: When false, this job will wait in a PENDING state after
> > it has
> > # finished its work, waiting for @block-job-finalize before
> > @@ -2249,7 +2250,8 @@
> > # @filter-node-name: the node name that should be assigned to the
> > #filter driver that the mirror job inserts into the
> > graph
> > #above @device. If this option is not given, a node
> > name is
> > -#autogenerated. (Since: 2.9)
> > +#autogenerated. Omitting this option is deprecated, it
> > will
> > +#be required in future. (Since: 2.9)
> > #
> > # @copy-mode: when to copy data to the destination; defaults to
> > 'background'
> > # (Since: 3.0)
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 3aa1e832a8..624da0b4a2 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -762,7 +762,15 @@ struct BlockDriverState {
> > bool sg;/* if true, the device is a /dev/sg* */
> > bool probed;/* if true, format was probed rather than specified */
> > bool force_share; /* if true, always allow all shared permissions */
> > -bool implicit; /* if true, this filter node was automatically
> > inserted */
> > +
> > +/*
> > + * @implicit field is deprecated, don't set it to true for new filters.
> > + * If true, this filter node was automatically inserted and user don't
> > + * know about it and unprepared for any effects of it. So, implicit
> > + * filters are workarounded and skipped in many places of the block
> > + * layer code.
> > + */
> > +bool implicit;
> >
> > BlockDriver *drv; /* NULL means no media */
> > void *opaque;
> > diff --git a/blockdev.c b/blockdev.c
> > index 36e9368e01..b3cfaccce1 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
> > *job_id, const char *device,
> > BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> > int job_flags = JOB_DEFAULT;
> >
> > +if (!has_filter_node_name) {
> > +warn_report("Omitting filter-node-name parameter is deprecated, it
> > "
> > +"will be required in future");
> > +}
> > +
> > if (!has_speed) {
> > speed = 0;
> > }
> > @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
> > *job_id,
> > Error *local_err = NULL;
> > int ret;
> >
> > +if (!has_filter_node_name) {
> > +warn_report("Omitting filter-node-name parameter is deprecated, it
> > "
> > +"will be required in future");
> > +}
> > +
> > bs = qmp_get_root_bs(device, errp);
> > if (!bs) {
> > return;
> >
>
> This might be OK to do right away, though.
>
> I asked Markus this not too long ago; do we want to amend the QAPI
> schema specification to allow
Re: [libvirt] [PATCH 5/5] util: default to read-only in virPCIDeviceConfigOpen
On Tue, Aug 13, 2019 at 03:45:19PM +0200, Ján Tomko wrote: > All the callers left require virPCIDeviceConfigOpen to be fatal > and only use read-only access to the config file. > > Signed-off-by: Ján Tomko > --- FYI this patch broke make check, both virpcitest and virhostdevtest. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] util: default to read-only in virPCIDeviceConfigOpen
On Thu, Aug 15, 2019 at 10:10:06 +0200, Erik Skultety wrote: > On Tue, Aug 13, 2019 at 03:45:19PM +0200, Ján Tomko wrote: > > All the callers left require virPCIDeviceConfigOpen to be fatal > > and only use read-only access to the config file. > > > > Signed-off-by: Ján Tomko > > --- > > FYI this patch broke make check, both virpcitest and virhostdevtest. Actually it's the previous patch according to my bisection and it happens only with gcc. Clang works fine. This hints to some inlining weridness happening together with the pci mocking code. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] docs: Make anchors in API html files clickable/linkable
Use 'id' instead of 'name' for anchors which adds the hidden clickable headerlink helper so it's way simpler to link to a specific part of the docs. Signed-off-by: Peter Krempa --- docs/newapi.xsl | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/newapi.xsl b/docs/newapi.xsl index c808fe5ff8..f9cbcb6323 100644 --- a/docs/newapi.xsl +++ b/docs/newapi.xsl @@ -308,7 +308,7 @@ - + enum @@ -357,7 +357,7 @@ - + struct @@ -447,7 +447,7 @@ - + #define @@ -558,7 +558,7 @@ - + typedef @@ -636,7 +636,7 @@ - + -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: storage: Fix parsing of 'exportname' from legacy NBD strings
On Wed, Jul 31, 2019 at 17:22:48 +0200, Peter Krempa wrote: > If the nbd export name contains a colon, our parser would not parse it > properly as we split the string by colons. Modify the code to look up > the exportname and copy any trailing characters as the export name is > supposed to be at the end of the string. > > https://bugzilla.redhat.com/show_bug.cgi?id=1733044 > > Signed-off-by: Peter Krempa > --- > src/util/virstoragefile.c | 6 -- > tests/virstoragetest.c| 8 > 2 files changed, 12 insertions(+), 2 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>
>
> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> > To get rid of implicit filters related workarounds in future let's
> > deprecate them now.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > ---
> > qemu-deprecated.texi | 7 +++
> > qapi/block-core.json | 6 --
> > include/block/block_int.h | 10 +-
> > blockdev.c| 10 ++
> > 4 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index 2753fafd0b..8222440148 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets
> > in server mode
> >
> > Use blockdev-mirror and blockdev-backup instead.
> >
> > +@subsection implicit filters (since 4.2)
> > +
> > +Mirror and commit jobs inserts filters, which becomes implicit if user
> > +omitted filter-node-name parameter. So omitting it is deprecated, set it
> > +always. Note, that drive-mirror don't have this parameter, so it will
> > +create implicit filter anyway, but drive-mirror is deprecated itself too.
> > +
> > @section Human Monitor Protocol (HMP) commands
> >
> > @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
> > (since 3.1)
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 4e35526634..0505ac9d8b 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1596,7 +1596,8 @@
> > # @filter-node-name: the node name that should be assigned to the
> > #filter driver that the commit job inserts into the
> > graph
> > #above @top. If this option is not given, a node name
> > is
> > -#autogenerated. (Since: 2.9)
> > +#autogenerated. Omitting this option is deprecated, it
> > will
> > +#be required in future. (Since: 2.9)
> > #
> > # @auto-finalize: When false, this job will wait in a PENDING state after
> > it has
> > # finished its work, waiting for @block-job-finalize before
> > @@ -2249,7 +2250,8 @@
> > # @filter-node-name: the node name that should be assigned to the
> > #filter driver that the mirror job inserts into the
> > graph
> > #above @device. If this option is not given, a node
> > name is
> > -#autogenerated. (Since: 2.9)
> > +#autogenerated. Omitting this option is deprecated, it
> > will
> > +#be required in future. (Since: 2.9)
> > #
> > # @copy-mode: when to copy data to the destination; defaults to
> > 'background'
> > # (Since: 3.0)
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 3aa1e832a8..624da0b4a2 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -762,7 +762,15 @@ struct BlockDriverState {
> > bool sg;/* if true, the device is a /dev/sg* */
> > bool probed;/* if true, format was probed rather than specified */
> > bool force_share; /* if true, always allow all shared permissions */
> > -bool implicit; /* if true, this filter node was automatically
> > inserted */
> > +
> > +/*
> > + * @implicit field is deprecated, don't set it to true for new filters.
> > + * If true, this filter node was automatically inserted and user don't
> > + * know about it and unprepared for any effects of it. So, implicit
> > + * filters are workarounded and skipped in many places of the block
> > + * layer code.
> > + */
> > +bool implicit;
> >
> > BlockDriver *drv; /* NULL means no media */
> > void *opaque;
> > diff --git a/blockdev.c b/blockdev.c
> > index 36e9368e01..b3cfaccce1 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
> > *job_id, const char *device,
> > BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> > int job_flags = JOB_DEFAULT;
> >
> > +if (!has_filter_node_name) {
> > +warn_report("Omitting filter-node-name parameter is deprecated, it
> > "
> > +"will be required in future");
> > +}
> > +
> > if (!has_speed) {
> > speed = 0;
> > }
> > @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
> > *job_id,
> > Error *local_err = NULL;
> > int ret;
> >
> > +if (!has_filter_node_name) {
> > +warn_report("Omitting filter-node-name parameter is deprecated, it
> > "
> > +"will be required in future");
> > +}
> > +
> > bs = qmp_get_root_bs(device, errp);
> > if (!bs) {
> > return;
> >
>
> This might be OK to do right away, though.
>
> I asked Markus this not too long ago; do we want to amend the QAPI
> schema specification to allow
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
On 8/15/19 6:49 AM, Kevin Wolf wrote:
> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>>
>>
>> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> To get rid of implicit filters related workarounds in future let's
>>> deprecate them now.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> qemu-deprecated.texi | 7 +++
>>> qapi/block-core.json | 6 --
>>> include/block/block_int.h | 10 +-
>>> blockdev.c| 10 ++
>>> 4 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>>> index 2753fafd0b..8222440148 100644
>>> --- a/qemu-deprecated.texi
>>> +++ b/qemu-deprecated.texi
>>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets
>>> in server mode
>>>
>>> Use blockdev-mirror and blockdev-backup instead.
>>>
>>> +@subsection implicit filters (since 4.2)
>>> +
>>> +Mirror and commit jobs inserts filters, which becomes implicit if user
>>> +omitted filter-node-name parameter. So omitting it is deprecated, set it
>>> +always. Note, that drive-mirror don't have this parameter, so it will
>>> +create implicit filter anyway, but drive-mirror is deprecated itself too.
>>> +
>>> @section Human Monitor Protocol (HMP) commands
>>>
>>> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
>>> (since 3.1)
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 4e35526634..0505ac9d8b 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1596,7 +1596,8 @@
>>> # @filter-node-name: the node name that should be assigned to the
>>> #filter driver that the commit job inserts into the
>>> graph
>>> #above @top. If this option is not given, a node name
>>> is
>>> -#autogenerated. (Since: 2.9)
>>> +#autogenerated. Omitting this option is deprecated, it
>>> will
>>> +#be required in future. (Since: 2.9)
>>> #
>>> # @auto-finalize: When false, this job will wait in a PENDING state after
>>> it has
>>> # finished its work, waiting for @block-job-finalize before
>>> @@ -2249,7 +2250,8 @@
>>> # @filter-node-name: the node name that should be assigned to the
>>> #filter driver that the mirror job inserts into the
>>> graph
>>> #above @device. If this option is not given, a node
>>> name is
>>> -#autogenerated. (Since: 2.9)
>>> +#autogenerated. Omitting this option is deprecated, it
>>> will
>>> +#be required in future. (Since: 2.9)
>>> #
>>> # @copy-mode: when to copy data to the destination; defaults to
>>> 'background'
>>> # (Since: 3.0)
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 3aa1e832a8..624da0b4a2 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -762,7 +762,15 @@ struct BlockDriverState {
>>> bool sg;/* if true, the device is a /dev/sg* */
>>> bool probed;/* if true, format was probed rather than specified */
>>> bool force_share; /* if true, always allow all shared permissions */
>>> -bool implicit; /* if true, this filter node was automatically
>>> inserted */
>>> +
>>> +/*
>>> + * @implicit field is deprecated, don't set it to true for new filters.
>>> + * If true, this filter node was automatically inserted and user don't
>>> + * know about it and unprepared for any effects of it. So, implicit
>>> + * filters are workarounded and skipped in many places of the block
>>> + * layer code.
>>> + */
>>> +bool implicit;
>>>
>>> BlockDriver *drv; /* NULL means no media */
>>> void *opaque;
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 36e9368e01..b3cfaccce1 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
>>> *job_id, const char *device,
>>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>>> int job_flags = JOB_DEFAULT;
>>>
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>> if (!has_speed) {
>>> speed = 0;
>>> }
>>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
>>> *job_id,
>>> Error *local_err = NULL;
>>> int ret;
>>>
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>> bs = qmp_get_root_bs(device, errp);
>>> if (!bs) {
>>> return;
>>>
>>
>> This might be OK to do right away, though.
>>
>> I asked Markus this not too long ago; do we want to amend
Re: [libvirt] [PATCH 2/2] virpcimock: Mock __open_2()
On Thu, Aug 15, 2019 at 05:15:57PM +0200, Michal Privoznik wrote: > Hold on to your hat, this is going to be a wild ride. As nearly > nothing in glic, nor open() is a real function. Just look into > bits/fcntl2.h and you'll see that open() is actually a thin > wrapper that calls either __open_alias() or __open_2(). Now, > before 801ebb5edb6 the open() done in > virPCIDeviceConfigOpenInternal() had a constant oflags (we were > opening the pci config with O_RDWR). And since we were not > passing any mode nor O_CREAT the wrapper decided to call > __open_alias() which was open() provided by our mock. So far so > good. But after the referenced commit, the oflags is no longer > compile time constant and therefore the wrapper calls __open_2() > which we don't mock and thus the real __open_2() from glibc was > called and thus we did try to open real path from host's /sys. > This of course fails with variety of errors. > > Signed-off-by: Michal Privoznik > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] ci: Allow gdb in containers
On Thu, Aug 15, 2019 at 05:15:56PM +0200, Michal Privoznik wrote: > The gdb requires ptrace capability, but the way we run containers > now is that they drop every capability. Preserve SYS_PTRACE then. > > Signed-off-by: Michal Privoznik > --- Makes sense to me, so I can give you my: Reviewed-by: Erik Skultety ...but I'd wait and let others to comment. > Makefile.ci | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Makefile.ci b/Makefile.ci > index 8857c953b2..977e0445c6 100644 > --- a/Makefile.ci > +++ b/Makefile.ci > @@ -167,6 +167,7 @@ CI_ENGINE_ARGS = \ > --volume $(CI_HOST_SRCDIR):$(CI_CONT_SRCDIR):z \ > --workdir $(CI_CONT_SRCDIR) \ > --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ > + --cap-add=SYS_PTRACE \ > $(NULL) > > ci-check-engine: > -- > 2.21.0 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Am 15.08.2019 um 18:07 hat John Snow geschrieben:
>
>
> On 8/15/19 6:49 AM, Kevin Wolf wrote:
> > Am 14.08.2019 um 21:27 hat John Snow geschrieben:
> >>
> >>
> >> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> >>> To get rid of implicit filters related workarounds in future let's
> >>> deprecate them now.
> >>>
> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >>> ---
> >>> qemu-deprecated.texi | 7 +++
> >>> qapi/block-core.json | 6 --
> >>> include/block/block_int.h | 10 +-
> >>> blockdev.c| 10 ++
> >>> 4 files changed, 30 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> >>> index 2753fafd0b..8222440148 100644
> >>> --- a/qemu-deprecated.texi
> >>> +++ b/qemu-deprecated.texi
> >>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to
> >>> sockets in server mode
> >>>
> >>> Use blockdev-mirror and blockdev-backup instead.
> >>>
> >>> +@subsection implicit filters (since 4.2)
> >>> +
> >>> +Mirror and commit jobs inserts filters, which becomes implicit if user
> >>> +omitted filter-node-name parameter. So omitting it is deprecated, set it
> >>> +always. Note, that drive-mirror don't have this parameter, so it will
> >>> +create implicit filter anyway, but drive-mirror is deprecated itself too.
> >>> +
> >>> @section Human Monitor Protocol (HMP) commands
> >>>
> >>> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
> >>> (since 3.1)
> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >>> index 4e35526634..0505ac9d8b 100644
> >>> --- a/qapi/block-core.json
> >>> +++ b/qapi/block-core.json
> >>> @@ -1596,7 +1596,8 @@
> >>> # @filter-node-name: the node name that should be assigned to the
> >>> #filter driver that the commit job inserts into the
> >>> graph
> >>> #above @top. If this option is not given, a node
> >>> name is
> >>> -#autogenerated. (Since: 2.9)
> >>> +#autogenerated. Omitting this option is deprecated,
> >>> it will
> >>> +#be required in future. (Since: 2.9)
> >>> #
> >>> # @auto-finalize: When false, this job will wait in a PENDING state
> >>> after it has
> >>> # finished its work, waiting for @block-job-finalize
> >>> before
> >>> @@ -2249,7 +2250,8 @@
> >>> # @filter-node-name: the node name that should be assigned to the
> >>> #filter driver that the mirror job inserts into the
> >>> graph
> >>> #above @device. If this option is not given, a node
> >>> name is
> >>> -#autogenerated. (Since: 2.9)
> >>> +#autogenerated. Omitting this option is deprecated,
> >>> it will
> >>> +#be required in future. (Since: 2.9)
> >>> #
> >>> # @copy-mode: when to copy data to the destination; defaults to
> >>> 'background'
> >>> # (Since: 3.0)
> >>> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >>> index 3aa1e832a8..624da0b4a2 100644
> >>> --- a/include/block/block_int.h
> >>> +++ b/include/block/block_int.h
> >>> @@ -762,7 +762,15 @@ struct BlockDriverState {
> >>> bool sg;/* if true, the device is a /dev/sg* */
> >>> bool probed;/* if true, format was probed rather than specified
> >>> */
> >>> bool force_share; /* if true, always allow all shared permissions */
> >>> -bool implicit; /* if true, this filter node was automatically
> >>> inserted */
> >>> +
> >>> +/*
> >>> + * @implicit field is deprecated, don't set it to true for new
> >>> filters.
> >>> + * If true, this filter node was automatically inserted and user
> >>> don't
> >>> + * know about it and unprepared for any effects of it. So, implicit
> >>> + * filters are workarounded and skipped in many places of the block
> >>> + * layer code.
> >>> + */
> >>> +bool implicit;
> >>>
> >>> BlockDriver *drv; /* NULL means no media */
> >>> void *opaque;
> >>> diff --git a/blockdev.c b/blockdev.c
> >>> index 36e9368e01..b3cfaccce1 100644
> >>> --- a/blockdev.c
> >>> +++ b/blockdev.c
> >>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
> >>> *job_id, const char *device,
> >>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> >>> int job_flags = JOB_DEFAULT;
> >>>
> >>> +if (!has_filter_node_name) {
> >>> +warn_report("Omitting filter-node-name parameter is deprecated,
> >>> it "
> >>> +"will be required in future");
> >>> +}
> >>> +
> >>> if (!has_speed) {
> >>> speed = 0;
> >>> }
> >>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const
> >>> char *job_id,
> >>> Error *local_err = NULL;
> >>> int ret;
> >>>
> >>> +if (!has_filter_node_name) {
> >>> +warn_report("Omitting filter-node-name
[libvirt] [PATCH 1/2] ci: Allow gdb in containers
The gdb requires ptrace capability, but the way we run containers now is that they drop every capability. Preserve SYS_PTRACE then. Signed-off-by: Michal Privoznik --- Makefile.ci | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile.ci b/Makefile.ci index 8857c953b2..977e0445c6 100644 --- a/Makefile.ci +++ b/Makefile.ci @@ -167,6 +167,7 @@ CI_ENGINE_ARGS = \ --volume $(CI_HOST_SRCDIR):$(CI_CONT_SRCDIR):z \ --workdir $(CI_CONT_SRCDIR) \ --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ + --cap-add=SYS_PTRACE \ $(NULL) ci-check-engine: -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Fix the latest virpcitest failure
See 2/2 for explanation. Michal Prívozník (2): ci: Allow gdb in containers virpcimock: Mock __open_2() Makefile.ci| 1 + tests/virpcimock.c | 29 + 2 files changed, 30 insertions(+) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] virpcimock: Mock __open_2()
Hold on to your hat, this is going to be a wild ride. As nearly
nothing in glic, nor open() is a real function. Just look into
bits/fcntl2.h and you'll see that open() is actually a thin
wrapper that calls either __open_alias() or __open_2(). Now,
before 801ebb5edb6 the open() done in
virPCIDeviceConfigOpenInternal() had a constant oflags (we were
opening the pci config with O_RDWR). And since we were not
passing any mode nor O_CREAT the wrapper decided to call
__open_alias() which was open() provided by our mock. So far so
good. But after the referenced commit, the oflags is no longer
compile time constant and therefore the wrapper calls __open_2()
which we don't mock and thus the real __open_2() from glibc was
called and thus we did try to open real path from host's /sys.
This of course fails with variety of errors.
Signed-off-by: Michal Privoznik
---
tests/virpcimock.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index beb5e1490d..829d61cd3f 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -32,6 +32,7 @@
static int (*real_access)(const char *path, int mode);
static int (*real_open)(const char *path, int flags, ...);
+static int (*real___open_2)(const char *path, int flags);
static int (*real_close)(int fd);
static DIR * (*real_opendir)(const char *name);
static char *(*real_virFileCanonicalizePath)(const char *path);
@@ -805,6 +806,7 @@ init_syms(void)
VIR_MOCK_REAL_INIT(access);
VIR_MOCK_REAL_INIT(open);
+VIR_MOCK_REAL_INIT(__open_2);
VIR_MOCK_REAL_INIT(close);
VIR_MOCK_REAL_INIT(opendir);
VIR_MOCK_REAL_INIT(virFileCanonicalizePath);
@@ -932,6 +934,33 @@ open(const char *path, int flags, ...)
return ret;
}
+
+int
+__open_2(const char *path, int flags)
+{
+VIR_AUTOFREE(char *) newpath = NULL;
+int ret;
+
+init_syms();
+
+if (STRPREFIX(path, SYSFS_PCI_PREFIX) &&
+getrealpath(, path) < 0)
+return -1;
+
+ret = real___open_2(newpath ? newpath : path, flags);
+
+/* Catch both: /sys/bus/pci/drivers/... and
+ * /sys/bus/pci/device/.../driver/... */
+if (ret >= 0 && STRPREFIX(path, SYSFS_PCI_PREFIX) &&
+strstr(path, "driver") && add_fd(ret, path) < 0) {
+real_close(ret);
+ret = -1;
+}
+
+return ret;
+}
+
+
DIR *
opendir(const char *path)
{
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
On 8/15/19 12:48 PM, Kevin Wolf wrote:
> Am 15.08.2019 um 18:07 hat John Snow geschrieben:
>> On 8/15/19 6:49 AM, Kevin Wolf wrote:
>>> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
This might be OK to do right away, though.
I asked Markus this not too long ago; do we want to amend the QAPI
schema specification to allow commands to return with "Warning" strings,
or "Deprecated" stings to allow in-band deprecation notices for cases
like these?
example:
{ "return": {},
"deprecated": True,
"warning": "Omitting filter-node-name parameter is deprecated, it will
be required in the future"
}
There's no "error" key, so this should be recognized as success by
compatible clients, but they'll definitely see the extra information.
Part of my motivation is to facilitate a more aggressive deprecation of
legacy features by ensuring that we are able to rigorously notify users
through any means that they need to adjust their scripts.
>>>
>>> Who would read this, though? In the best case it ends up deep in a
>>> libvirt log that nobody will look at because there was no error. In the
>>> more common case, the debug level is configured so that QMP traffic
>>> isn't even logged.
>>>
>>> Kevin
>>>
>>
>> I believe you are right, but I also can't shake the feeling that this
>> attitude ensures that we'll never find a way to expose this information
>> to the end-user. Is this not too defeatist?
>
> I think the discussed approach that seemed most likely to me to succeed
> was adding a command line option that makes QEMU just crash if you use a
> deprecated feature, and enable that in libvirt test cases (or possibly
> even any non-release builds, though maybe it's a bit harsh there).
>
>> I think deprecation notices in the QMP stream has two benefits:
>>
>> 1) Any direct usages via qmp-shell or manual JSON connection are likely
>> to see this message in development or testing. I feel the usage of QEMU
>> directly is more likely to increase with time as other stacks seek to
>> work around libvirt.
>>
>> [Whether or not they should is another question, but I believe the
>> current reality to be that people are trying to.]
>
> I don't know about other people, but as a human user, I don't care about
> deprecation notices. As long as something works, I use it, and once I
> get an error message back, I'll use something else.
>
> If I manually enter drive_mirror and get a warning back, that doesn't
> tell me that libvirt still does the same thing and needs to be fixed. It
> just tells me that in the future I might need to change the commands
> that I use manually.
>
That the message we return needs to be *useful* doesn't sound like a
count against it.
> I guess this would still prevent adding new libvirt features that build
> on deprecated QEMU features because some manual testing will be involved
> there. But was this ever a problem?
>
No, because until recently we didn't deprecate anything.
>> 2) Programmatic deprecation notices can't be presented to a user at all
>> if we don't send them; at least this way it becomes libvirt's problem
>> over what to do with them. Perhaps even just in testing and regression
>> suites libvirt can assert that it sees no deprecation warnings (or
>> whitelist certain ones it knows about.)
>>
>> In the case of libvirt, it's not even necessarily about making sure the
>> end user sees it, because it isn't even necessarily the user's fault --
>> it's libvirt's. This is a sure-fire programmatic way to communicate
>> compatibility changes to libvirt.
>
> If libvirt uses this to make test cases fail, it could work.
>
Yeah, I think there's solid use there. I'll continue along in Markus's
thread.
> Kevin
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Exposing feature deprecation to machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters)
On 8/15/19 10:16 AM, Markus Armbruster wrote:
> John Snow writes:
>
>> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> To get rid of implicit filters related workarounds in future let's
>>> deprecate them now.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
> [...]
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 36e9368e01..b3cfaccce1 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
>>> *job_id, const char *device,
>>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>>> int job_flags = JOB_DEFAULT;
>>>
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>> if (!has_speed) {
>>> speed = 0;
>>> }
>>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
>>> *job_id,
>>> Error *local_err = NULL;
>>> int ret;
>>>
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>> bs = qmp_get_root_bs(device, errp);
>>> if (!bs) {
>>> return;
>>>
>>
>> This might be OK to do right away, though.
>>
>> I asked Markus this not too long ago; do we want to amend the QAPI
>> schema specification to allow commands to return with "Warning" strings,
>> or "Deprecated" stings to allow in-band deprecation notices for cases
>> like these?
>>
>> example:
>>
>> { "return": {},
>> "deprecated": True,
>> "warning": "Omitting filter-node-name parameter is deprecated, it will
>> be required in the future"
>> }
>>
>> There's no "error" key, so this should be recognized as success by
>> compatible clients, but they'll definitely see the extra information.
>
> This is a compatible evolution of the QMP protocol.
>
>> Part of my motivation is to facilitate a more aggressive deprecation of
>> legacy features by ensuring that we are able to rigorously notify users
>> through any means that they need to adjust their scripts.
>
> Yes, we should help libvirt etc. with detecting use of deprecated
> features. We discussed this at the KVM Forum 2018 BoF on deprecating
> stuff. Minutes:
>
> Message-ID: <87mur0ls8o@dusky.pond.sub.org>
> https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
>
> Last item is relevant here.
>
> Adding deprecation information to QMP's success response belongs to "We
> can also pass the buck to the next layer up", next to "emit a QMP
> event".
>
> Let's compare the two, i.e. "deprecation info in success response"
> vs. "deprecation event".
>
> 1. Possible triggers
>
> Anything we put in the success response should only ever apply to the
> (successful) command. So this one's limited to QMP commands.
>
> A QMP event is not limited to QMP commands. For instance, it could be
> emitted for deprecated CLI features (long after the fact, in addition to
> human-readable warnings on stderr), or when we detect use of a
> deprecated feature only after we sent the success response, say in a
> job. Neither use case is particularly convincing. Reporting use of
> deprecated CLI in QMP feels like a work-around for the CLI's
> machine-unfriendliness. Job-like commands should really check their
> arguments upfront.
>
> 2. Connection to trigger
>
> Connecting responses to commands is the QMP protocol's responsibility.
> Transmitting deprecation information in the response trivially ties it
> to the offending command.
>
> The QMP protocol doesn't tie events to anything. Thus, a deprecation
> event needs an event-specific tie to its trigger.
>
> The obvious way to tie it to a command mirrors how the QMP protocol ties
> responses to commands: by command ID. The event either has to be sent
> just to the offending monitor (currently, all events are broadcast to
> all monitors), or include a suitable monitor ID.
>
> For non-command triggers, we'd have to invent some other tie.
>
> 3. Interface complexity
>
> Tying the event to some arbitrary trigger adds complexity.
>
> Do we need non-command triggers, and badly enough to justify the
> additional complexity?
>
> 4. Implementation complexity
>
> Emitting an event could be as simple as
>
> qapi_event_send_deprecated(qmp_command_id(),
>"Omitting 'filter-node-name'");
>
> where qmp_command_id() returns the ID of the currently executing
> command. Making qmp_command_id() work is up to the QMP core. Simple
> enough as long as each QMP command runs to completion before its monitor
> starts the next one.
>
> The event is "fire and forget". There is no warning object propagated
> up the call chain into the QMP core like errors objects are.
>
> "Fire and forget" is ideal for letting arbitrary code decide "this is
>
[libvirt] [PATCH] test_driver: implement virConnectCompareCPU
Signed-off-by: Ilias Stamatis
---
Probably we need to extend the host CPU's capabilities as well along
with this patch. Currently virCPUx86Compare reports "unknown host CPU".
src/test/test_driver.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index c39eef2d4b..b1037f4eab 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1529,6 +1529,26 @@ static int testConnectGetMaxVcpus(virConnectPtr conn
ATTRIBUTE_UNUSED,
return 32;
}
+
+static int
+testConnectCompareCPU(virConnectPtr conn,
+ const char *xmlDesc,
+ unsigned int flags)
+{
+testDriverPtr privconn = conn->privateData;
+bool failIncompatible = !!(flags &
VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE);
+int ret = VIR_CPU_COMPARE_ERROR;
+
+virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE,
+ VIR_CPU_COMPARE_ERROR);
+
+ret = virCPUCompareXML(privconn->caps->host.arch, privconn->caps->host.cpu,
+ xmlDesc, failIncompatible);
+
+return ret;
+}
+
+
static char *
testConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED,
const char **xmlCPUs,
@@ -9510,6 +9530,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainRevertToSnapshot = testDomainRevertToSnapshot, /* 1.1.4 */
.domainSnapshotDelete = testDomainSnapshotDelete, /* 1.1.4 */
+.connectCompareCPU = testConnectCompareCPU, /* 5.7.0 */
.connectBaselineCPU = testConnectBaselineCPU, /* 1.2.0 */
.domainCheckpointCreateXML = testDomainCheckpointCreateXML, /* 5.6.0 */
.domainCheckpointGetXMLDesc = testDomainCheckpointGetXMLDesc, /* 5.6.0 */
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virDomainGetAutostart takes a pointer that it writes the output value to
The current version of get_autostart seg faults. This patch
correctly passes a pointer to an int to virDomainGetAutostart
and returns a result based on the value of that int
Signed-off-by: sage Imel
---
src/domain.rs | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/domain.rs b/src/domain.rs
index 11ecb3c..acb9e6e 100644
--- a/src/domain.rs
+++ b/src/domain.rs
@@ -136,7 +136,7 @@ extern "C" {
fn virDomainGetHostname(ptr: sys::virDomainPtr, flags: libc::c_uint) ->
*mut libc::c_char;
fn virDomainGetUUIDString(ptr: sys::virDomainPtr, uuid: *mut libc::c_char)
-> libc::c_int;
fn virDomainGetXMLDesc(ptr: sys::virDomainPtr, flags: libc::c_uint) ->
*mut libc::c_char;
-fn virDomainGetAutostart(ptr: sys::virDomainPtr) -> libc::c_int;
+fn virDomainGetAutostart(ptr: sys::virDomainPtr, autostart: *mut
libc::c_int) -> libc::c_int;
fn virDomainSetAutostart(ptr: sys::virDomainPtr, autostart: libc::c_uint)
-> libc::c_int;
fn virDomainGetID(ptr: sys::virDomainPtr) -> libc::c_uint;
fn virDomainSetMaxMemory(ptr: sys::virDomainPtr, memory: libc::c_ulong) ->
libc::c_int;
@@ -1036,11 +1036,12 @@ impl Domain {
pub fn get_autostart() -> Result {
unsafe {
-let ret = virDomainGetAutostart(self.as_ptr());
+let mut autostart: libc::c_int = 0;
+let ret = virDomainGetAutostart(self.as_ptr(), autostart);
if ret == -1 {
return Err(Error::new());
}
-return Ok(ret == 1);
+return Ok(autostart == 1);
}
}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 1/2] qapi: deprecate drive-mirror and drive-backup
On 8/15/19 3:44 AM, Peter Krempa wrote:
> On Wed, Aug 14, 2019 at 15:22:15 -0400, John Snow wrote:
>>
>>
>> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> It's hard and not necessary to maintain outdated versions of these
>>> commands.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> qemu-deprecated.texi | 4
>>> qapi/block-core.json | 4
>>> qapi/transaction.json | 2 +-
>>> blockdev.c| 10 ++
>>> 4 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>>> index fff07bb2a3..2753fafd0b 100644
>>> --- a/qemu-deprecated.texi
>>> +++ b/qemu-deprecated.texi
>>> @@ -179,6 +179,10 @@ and accurate ``query-qmp-schema'' command.
>>> Character devices creating sockets in client mode should not specify
>>> the 'wait' field, which is only applicable to sockets in server mode
>>>
>>> +@subsection drive-mirror, drive-backup and drive-backup transaction action
>>> (since 4.2)
>>> +
>>> +Use blockdev-mirror and blockdev-backup instead.
>>> +
>>> @section Human Monitor Protocol (HMP) commands
>>>
>>> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
>>> (since 3.1)
>
> [...]
>
>>> @@ -3831,6 +3838,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>>> const char *format = arg->format;
>>> int ret;
>>>
>>> +warn_report("drive-mirror command is deprecated and will disappear in "
>>> +"future. Use blockdev-mirror instead");
>>> +
>>> bs = qmp_get_root_bs(arg->device, errp);
>>> if (!bs) {
>>> return;
>>>
>>
>> Hm!
>>
>> I wonder if this is ever-so-slightly too soon for our friends over at
>> the libvirt project.
>>
>> I don't think they have fully moved away from the non-blockdev
>> interfaces *just yet*, and I might encourage seeing the first full
>> libvirt release that does support and use it before we start the
>> deprecation clock.
>>
>> (Jst in case.)
>>
>> That's just me being very, very cautious though.
>>
>> Peter Krempa, how do you feel about this?
>
> Thanks for the heads up!
>
You're welcome!
> Currently libvirt does not use 'drive-backup' at all so that one can be
> deprecated immediately.
>
> In case of 'drive-mirror' the situation is a bit more complex:
>
> Libvirt uses 'drive-mirror' currently in the following places
>
> 1) virDomainBlockCopy API
> With blockdev integration enabled this will go away. Pathces are being
> reviewed:
>
> https://www.redhat.com/archives/libvir-list/2019-August/msg00295.html
>
> 2) VM migration with non-shared storage
> Currently uses 'drive-mirror' in most cases but there is pre-existing
> integration for blockdev-mirror for nbd+tls. I need to make sure that
> the blockdev version will be used unconditionally once the integration
> is enabled. This is a TODO.
>
> There is also one gotcha. In case when an 'sd' card device is used for
> the VM, libvirt disables all of blockdev, because SD cards can't be
> expressed with blockdev. There's too many code paths which would need
> checking to be worth it. To be fair, I'm not even sure when a sd card
> can be emulated by qemu as all of my basic tests failed and I did not
> care more.
>
> For libvirt to enable blockdev there's one more part missing and that's
> snapshot integration. I'm currently testing patches to integrate it with
> external snapshots, which should be posted soon.
>
> I also found a bug in qemu, which prevents creation of internal
> snapshots when -blockdev is used:
>
> When savevm HMP command is used (via QMP->HMP bridge) qemu invokes
> save_snapshot(), which calls bdrv_all_can_snapshot(). That function uses
> bdrv_next() to iterate all nodes which correspond to a block backend
> first, but then also iterates any other node which is monitor-owned.
>
> Since with blockdev all nodes including the ones for the 'file' protocol
> are monitor-owned, and 'file' does not support snapshots that check
> fails. A simple hack of skipping the second part in bdrv_next() allows
> to do a snapshot actually. Kevin told me that the idea is that also
> non-attached nodes should be considered for internal snapshod which is
> okay in my opinion, but given how the snapshot works for the files
> attached to backeds (and also in pre-blockdev use) only the top level of
> a chain should ever be considered for snapshot.
>
> So the summary is, that I'm pretty hopeful that we should be able to get
> rid of all reasonable uses of drive-mirror very soon after I finish
> snapshot integration. The only question is how much
> we care about SD card users being able to do a drive-mirror in the
> future.
>
OK. It sounds like we should hold off on deprecating these for now
because it's not certain which libvirt release will no longer need them,
but it sounds like it's hopefully not far off.
--js
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote_daemon_dispatch.c: typecast ARRAY_CARDINALITY() in remoteDispatchProbeURI()
On 8/14/19 7:13 AM, Michal Privoznik wrote: > Since users can enable/disable drivers at compile time, it may > happen that @drivers array is in fact empty (in both its > occurrences within the function). This means that > ARRAY_CARDINALITY() returns 0UL which makes gcc unhappy because > of loop condition: > >i < ARRAY_CARDINALITY(drivers) > > GCC complains that @i is unsigned and comparing an unsigned value > against 0 is always false. However, changing the type of @i to > ssize_t is not enough, because compiler still sees the unsigned > zero. The solution is to typecast the ARRAY_CARDINALITY(). > > Signed-off-by: Michal Privoznik > --- > src/remote/remote_daemon_dispatch.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Typing the original mail and reviewing and testing this patch probably took longer than fixing it in the first place :-). Thanks for taking care of it! Reviewed-by: Jim Fehlig Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] virpcimock: Mock __open_2()
On Thu, Aug 15, 2019 at 05:15:57PM +0200, Michal Privoznik wrote: Hold on to your hat, this is going to be a wild ride. As nearly nothing in glic, nor open() is a real function. Just look into s/glic/glibc/ bits/fcntl2.h and you'll see that open() is actually a thin wrapper that calls either __open_alias() or __open_2(). Now, before 801ebb5edb6 the open() done in virPCIDeviceConfigOpenInternal() had a constant oflags (we were opening the pci config with O_RDWR). And since we were not passing any mode nor O_CREAT the wrapper decided to call __open_alias() which was open() provided by our mock. So far so good. But after the referenced commit, the oflags is no longer compile time constant and therefore the wrapper calls __open_2() which we don't mock and thus the real __open_2() from glibc was called and thus we did try to open real path from host's /sys. This of course fails with variety of errors. Signed-off-by: Michal Privoznik --- tests/virpcimock.c | 29 + 1 file changed, 29 insertions(+) Thanks for tracking this down! Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] util: Export virStorageFileSupportsBackingChainTraversal
On Wed, Aug 14, 2019 at 06:59:16PM +0200, Peter Krempa wrote: The function will be reused in the qemu snapshot code. The argument is turned into const similarly to the other virStorageFileSupports* functions. Signed-off-by: Peter Krempa --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 4 ++-- src/util/virstoragefile.h | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] qemu: driver: Improve error suppression in qemuDomainStorageUpdatePhysical
On Wed, Aug 14, 2019 at 06:59:19PM +0200, Peter Krempa wrote:
None of the callers of qemuDomainStorageUpdatePhysical care about
errors.
Use the new flag for qemuDomainStorageOpenStat which suppresses some
errors and move the reset of the rest of the uncommon errors into this
function. Document what is happening in a comment for the function.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cc296c1fe3..2dffef9642 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12318,6 +12318,19 @@ qemuDomainStorageCloseStat(virStorageSourcePtr src,
}
+/**
+ * qemuDomainStorageUpdatePhysical:
+ * @driver: qemu driver
+ * @cfg: qemu driver configuration object
+ * @vm: domain object
+ * @src: storage source to update
+ *
+ * Update the physical size of the disk by reading the actual size of the image
+ * on disk.
+ *
+ * Returns 0 on successful update and -1 otherwise (some uncommon errors may be
+ * reported but are reset (thus only logged)).
By uncommon you mean missing network storage? :)
+ */
static int
qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg,
@@ -12331,8 +12344,11 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr
driver,
if (virStorageSourceIsEmpty(src))
return 0;
-if (qemuDomainStorageOpenStat(driver, cfg, vm, src, , , false) < 0)
+if ((ret = qemuDomainStorageOpenStat(driver, cfg, vm, src, , , true))
<= 0) {
+if (ret < 0)
+virResetLastError();
Still ugly to use virResetLastError, but at least it's an improvement.
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] util: storagefile: Don't report errors from virStorageSourceUpdatePhysicalSize
On Wed, Aug 14, 2019 at 06:59:18PM +0200, Peter Krempa wrote: virStorageSourceUpdatePhysicalSize is called only from qemuDomainStorageUpdatePhysical and all callers of it reset the libvirt error if -1 is returned. Don't bother setting the error in the first place. Signed-off-by: Peter Krempa --- src/util/virstoragefile.c | 12 ++-- 1 file changed, 2 insertions(+), 10 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] qemu: Allow skipping some errors in qemuDomainStorageOpenStat
On Wed, Aug 14, 2019 at 06:59:17PM +0200, Peter Krempa wrote: Some callers of this function actually don't care about errors and reset it. The message is still logged which might irritate users in this case. Add a boolean flag which will do few checks whether it actually makes sense to even try opening the storage file. For local files we check whether it exists and for remote files we at first see whether we even have a storage driver backend for it in the first place before trying to open it. Other problems will still report errors but these are the most common scenarios which can happen here. This patch changes the return value of the function so that the caller is able to differentiate the possibilities. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 20 +++- 1 file changed, 15 insertions(+), 5 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] access: fix incorrect addition to virAccessPermNetwork
Commit e69444e17 (first appeared in libvirt-5.5.0) added the new value
"VIR_ACCESS_PERM_NETWORK_SEARCH_PORTS" to the virAccessPerNetwork
enum, and also the string "search_ports" to the VIR_ENUM_IMPL() macro
for that enum. Unfortunately, the enum value was added in the middle
of the list, while the string was added to the end of the
VIR_ENUM_IMPL().
This patch corrects that error by moving the new value to the end of
the enum definition, so that the order matches that of the string
list.
Resolves: https://bugzilla.redhat.com/1741428
Signed-off-by: Laine Stump
---
src/access/viraccessperm.h | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h
index a42512d5e0..7480ee8c2f 100644
--- a/src/access/viraccessperm.h
+++ b/src/access/viraccessperm.h
@@ -410,18 +410,18 @@ typedef enum {
*/
VIR_ACCESS_PERM_NETWORK_START,
-/**
- * @desc: List network ports
- * @message: Listing network ports requires authorization
- */
-VIR_ACCESS_PERM_NETWORK_SEARCH_PORTS,
-
/**
* @desc: Stop network
* @message: Stopping network requires authorization
*/
VIR_ACCESS_PERM_NETWORK_STOP,
+/**
+ * @desc: List network ports
+ * @message: Listing network ports requires authorization
+ */
+VIR_ACCESS_PERM_NETWORK_SEARCH_PORTS,
+
VIR_ACCESS_PERM_NETWORK_LAST
} virAccessPermNetwork;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] qemu: Don't report some ignored errors in qemuDomainGetStatsOneBlockFallback
On Wed, Aug 14, 2019 at 06:59:21PM +0200, Peter Krempa wrote: The function ignores all errors from qemuStorageLimitsRefresh by calling virResetLastError. This still logs them. Since qemuStorageLimitsRefresh allows suppressing some, do so. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] qemu: Allow suppressing errors from qemuStorageLimitsRefresh
On Wed, Aug 14, 2019 at 06:59:20PM +0200, Peter Krempa wrote: qemuStorageLimitsRefresh uses qemuDomainStorageOpenStat internally and there are callers which don't care about the error. Propagate the skipInaccessible flag so that we can log less errors. Callers currently don't care about the return value change. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Kevin Wolf writes:
> Am 15.08.2019 um 18:07 hat John Snow geschrieben:
>>
>>
>> On 8/15/19 6:49 AM, Kevin Wolf wrote:
>> > Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>> >>
>> >>
>> >> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> >>> To get rid of implicit filters related workarounds in future let's
>> >>> deprecate them now.
>> >>>
>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> >>> ---
>> >>> qemu-deprecated.texi | 7 +++
>> >>> qapi/block-core.json | 6 --
>> >>> include/block/block_int.h | 10 +-
>> >>> blockdev.c| 10 ++
>> >>> 4 files changed, 30 insertions(+), 3 deletions(-)
>> >>>
>> >>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>> >>> index 2753fafd0b..8222440148 100644
>> >>> --- a/qemu-deprecated.texi
>> >>> +++ b/qemu-deprecated.texi
>> >>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to
>> >>> sockets in server mode
>> >>>
>> >>> Use blockdev-mirror and blockdev-backup instead.
>> >>>
>> >>> +@subsection implicit filters (since 4.2)
>> >>> +
>> >>> +Mirror and commit jobs inserts filters, which becomes implicit if user
>> >>> +omitted filter-node-name parameter. So omitting it is deprecated, set it
>> >>> +always. Note, that drive-mirror don't have this parameter, so it will
>> >>> +create implicit filter anyway, but drive-mirror is deprecated itself
>> >>> too.
>> >>> +
>> >>> @section Human Monitor Protocol (HMP) commands
>> >>>
>> >>> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
>> >>> (since 3.1)
>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >>> index 4e35526634..0505ac9d8b 100644
>> >>> --- a/qapi/block-core.json
>> >>> +++ b/qapi/block-core.json
>> >>> @@ -1596,7 +1596,8 @@
>> >>> # @filter-node-name: the node name that should be assigned to the
>> >>> #filter driver that the commit job inserts into the
>> >>> graph
>> >>> #above @top. If this option is not given, a node
>> >>> name is
>> >>> -#autogenerated. (Since: 2.9)
>> >>> +#autogenerated. Omitting this option is deprecated,
>> >>> it will
>> >>> +#be required in future. (Since: 2.9)
>> >>> #
>> >>> # @auto-finalize: When false, this job will wait in a PENDING state
>> >>> after it has
>> >>> # finished its work, waiting for @block-job-finalize
>> >>> before
>> >>> @@ -2249,7 +2250,8 @@
>> >>> # @filter-node-name: the node name that should be assigned to the
>> >>> #filter driver that the mirror job inserts into the
>> >>> graph
>> >>> #above @device. If this option is not given, a node
>> >>> name is
>> >>> -#autogenerated. (Since: 2.9)
>> >>> +#autogenerated. Omitting this option is deprecated,
>> >>> it will
>> >>> +#be required in future. (Since: 2.9)
>> >>> #
>> >>> # @copy-mode: when to copy data to the destination; defaults to
>> >>> 'background'
>> >>> # (Since: 3.0)
>> >>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> >>> index 3aa1e832a8..624da0b4a2 100644
>> >>> --- a/include/block/block_int.h
>> >>> +++ b/include/block/block_int.h
>> >>> @@ -762,7 +762,15 @@ struct BlockDriverState {
>> >>> bool sg;/* if true, the device is a /dev/sg* */
>> >>> bool probed;/* if true, format was probed rather than specified
>> >>> */
>> >>> bool force_share; /* if true, always allow all shared permissions */
>> >>> -bool implicit; /* if true, this filter node was automatically
>> >>> inserted */
>> >>> +
>> >>> +/*
>> >>> + * @implicit field is deprecated, don't set it to true for new
>> >>> filters.
>> >>> + * If true, this filter node was automatically inserted and user
>> >>> don't
>> >>> + * know about it and unprepared for any effects of it. So, implicit
>> >>> + * filters are workarounded and skipped in many places of the block
>> >>> + * layer code.
>> >>> + */
>> >>> +bool implicit;
>> >>>
>> >>> BlockDriver *drv; /* NULL means no media */
>> >>> void *opaque;
>> >>> diff --git a/blockdev.c b/blockdev.c
>> >>> index 36e9368e01..b3cfaccce1 100644
>> >>> --- a/blockdev.c
>> >>> +++ b/blockdev.c
>> >>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
>> >>> *job_id, const char *device,
>> >>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>> >>> int job_flags = JOB_DEFAULT;
>> >>>
>> >>> +if (!has_filter_node_name) {
>> >>> +warn_report("Omitting filter-node-name parameter is deprecated,
>> >>> it "
>> >>> +"will be required in future");
>> >>> +}
>> >>> +
>> >>> if (!has_speed) {
>> >>> speed = 0;
>> >>> }
>> >>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const
>> >>> char *job_id,
>> >>>
[libvirt] [PATCH] test_driver: Fix permissions for test_driver.c
From: Andrea Bolognani Introduced in commit 4a6ee53581b3. Signed-off-by: Andrea Bolognani (cherry picked from commit df1b5cf02efd4fee6f01ebe69fd0f1fd24b3947d) Reintroduced-by: fb275b76734ba1c0b18ad1088e3c82fb01961903 Signed-off-by: Ján Tomko --- src/test/test_driver.c | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 => 100644 src/test/test_driver.c diff --git a/src/test/test_driver.c b/src/test/test_driver.c old mode 100755 new mode 100644 -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] network: fix crash during cleanup from failure to allocate port
During networkPortCreateXML, if networkAllocatePort() failed,
networkReleasePort() would be called, which would (in the case of
network pools of macvtap passthrough devices) attempt to find the
allocated device by comparing port->plug.direct.linkdev to each device
in the pool. Since port->plug.direct.linkdev was still NULL, the
attempted strcmp would result in a SEGV.
Calling networkReleasePort() during error cleanup is something that
should only be done if networkAllocatePort() has already succeeded. It
turns out there is one other possible error exit from
networkPortCreateXML() that happens after networkAllocatePort() has
succeeded, so the code to call networkReleasePort() was just moved
down to there.
Resolves: https://bugzilla.redhat.com/1741390
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 1a5d08a00d..dae1def8de 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5592,20 +5592,20 @@ networkPortCreateXML(virNetworkPtr net,
rc = networkNotifyPort(obj, portdef);
else
rc = networkAllocatePort(obj, portdef);
-if (rc < 0) {
+if (rc < 0)
+goto cleanup;
+
+if (virNetworkObjAddPort(obj, portdef, driver->stateDir) < 0) {
virErrorPtr saved;
+
saved = virSaveLastError();
ignore_value(networkReleasePort(obj, portdef));
+virNetworkPortDefFree(portdef);
virSetError(saved);
virFreeError(saved);
goto cleanup;
}
-if (virNetworkObjAddPort(obj, portdef, driver->stateDir) < 0) {
-virNetworkPortDefFree(portdef);
-goto cleanup;
-}
-
ret = virGetNetworkPort(net, portdef->uuid);
cleanup:
virNetworkObjEndAPI();
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] network: replace virSaveLastError() with virErrorPreserveLast()
virErrorPreserveLast()/virErrorRestore() (added in commit 8333e7455
back in 2017), do a better better job of saving and restoring the last
libvirt error than virSaveLastError()/virErrorRestore() (they're
simpler, and they also save/restore the system errno).
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index dae1def8de..9059296e55 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2987,13 +2987,12 @@ networkStartNetwork(virNetworkDriverStatePtr driver,
cleanup:
if (ret < 0) {
+virErrorPtr save_err;
+
+virErrorPreserveLast(_err);
virNetworkObjUnsetDefTransient(obj);
-virErrorPtr save_err = virSaveLastError();
-int save_errno = errno;
networkShutdownNetwork(driver, obj);
-virSetError(save_err);
-virFreeError(save_err);
-errno = save_errno;
+virErrorRestore(_err);
}
return ret;
}
@@ -5596,13 +5595,13 @@ networkPortCreateXML(virNetworkPtr net,
goto cleanup;
if (virNetworkObjAddPort(obj, portdef, driver->stateDir) < 0) {
-virErrorPtr saved;
+virErrorPtr save_err;
-saved = virSaveLastError();
+virErrorPreserveLast(_err);
ignore_value(networkReleasePort(obj, portdef));
virNetworkPortDefFree(portdef);
-virSetError(saved);
-virFreeError(saved);
+virErrorRestore(_err);
+
goto cleanup;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] network: fix crash during cleanup from failure to allocate port
The first patch fixes the bug. The 2nd patch just updates some code that I noticed while fixing the bug (because I figured someone would whine that I was just moving around calls to outdated APIs). Laine Stump (2): network: fix crash during cleanup from failure to allocate port network: replace virSaveLastError() with virErrorPreserveLast() src/network/bridge_driver.c | 23 +++ 1 file changed, 11 insertions(+), 12 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
