Re: [PATCH v2 6/6] libvduse: Add support for reconnecting
On Tue, Mar 15, 2022 at 9:48 PM Stefan Hajnoczi wrote:
>
> On Tue, Feb 15, 2022 at 06:59:43PM +0800, Xie Yongji wrote:
> > +static int vduse_queue_inflight_get(VduseVirtq *vq, int desc_idx)
> > +{
> > +vq->log->inflight.desc[desc_idx].counter = vq->counter++;
> > +vq->log->inflight.desc[desc_idx].inflight = 1;
>
> Is a barrier needed between these two lines to prevent inflight = 1 with
> an undefined counter value?
Yes, I will fix it in v3.
Thanks,
Yongji
Re: [PATCH experiment 00/16] C++20 coroutine backend
On 3/15/22 16:55, Daniel P. Berrangé wrote: Expecting maintainers to enforce a subset during code review feels like it would be a tedious burden, that will inevitably let stuff through because humans are fallible, especially when presented with uninspiring, tedious, repetitive tasks. Restricting ourselves to a subset is only viable if we have an automated tool that can reliably enforce that subset. I'm not sure that any such tool exists, and not convinced our time is best served by trying to write & maintainer one either. We don't need to have a policy on which features are used. We need to have goals for what to use C++ for. I won't go into further details here, because I had already posted "When and how to use C++"[1] about an hour before your reply. IOW, I fear one we allow C++ in any level, it won't be practical to constrain it as much we desire. I fear us turning QEMU into even more of a monster like other big C++ apps I see which take all hours to compile while using all available RAM in Fedora RPM build hosts. Sorry but this is FUD. There's plenty of C++ apps and libraries that do not "take hours to compile while using all available RAM". You're probably thinking of the Chromium/Firefox/Libreoffice triplet but those are an order of magnitude larger than QEMU. And in fact, QEMU is *already* a monster that takes longer to compile than most other packages, no matter the language they're written in. Most of KDE and everything that uses Qt is written in C++, and so is Inkscape in GTK+ land. LLVM and Clang are written in C++. Hotspot and V8 are written in C++. Kodi, MAME and DolphinEmu are written in C++. GCC and GDB have migrated to C++ and their compile times have not exploded. My other question is whether adoption of C++ would complicate any desire to make more use of Rust in QEMU ? I know Rust came out of work by the Mozilla Firefox crew, and Firefox was C++, but I don't have any idea how they integrated use of Rust with Firefox, so whether there are any gotcha's for us or not ? Any Rust integration would go through C APIs. Using Rust in the block layer would certainly be much harder, though perhaps not impossible, if the block layer uses C++ coroutines. Rust supports something similar, but two-direction interoperability would be hard. For everything else, not much. Even if using C++, the fact that QEMU's APIs are primarily C would not change. Changing "timer_mod_ns(timer, ns)" to "timer.modify_ns(ns)" is not on the table. But really, first of all the question should be who is doing work on integrating Rust with QEMU. I typically hear about this topic exactly once a year at KVM Forum, and then nothing. We have seen Marc-André's QAPI integration experiment, but it's not clear to me what the path would be from there to wider use in QEMU. In particular, after ~3 years of talking about it, it is not even clear: - what subsystems would benefit the most from the adoption of Rust, and whether that would be feasible without a rewrite which will simply never happen - what the plans would be for coexistence of Rust and C code within a subsystem - whether maintainers would be on board with adopting a completely different language, and who in the community has enough Rust experience to shepherd us through the learning experience The first two questions have answers in the other message if s/Rust/C++/, and as to the last I think we're already further in the discussion. Thanks, Paolo
Re: [PATCH v2 1/6] block: Support passing NULL ops to blk_set_dev_ops()
On Tue, Mar 15, 2022 at 4:47 AM Stefan Hajnoczi wrote:
>
> On Mon, Mar 14, 2022 at 03:09:35PM -0400, John Snow wrote:
> > On Mon, Mar 14, 2022 at 1:23 PM Stefan Hajnoczi wrote:
> > >
> > > On Tue, Feb 15, 2022 at 06:59:38PM +0800, Xie Yongji wrote:
> > > > This supports passing NULL ops to blk_set_dev_ops()
> > > > so that we can remove stale ops in some cases.
> > > >
> > > > Signed-off-by: Xie Yongji
> > > > ---
> > > > block/block-backend.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > > index 4ff6b4d785..08dd0a3093 100644
> > > > --- a/block/block-backend.c
> > > > +++ b/block/block-backend.c
> > > > @@ -1015,7 +1015,7 @@ void blk_set_dev_ops(BlockBackend *blk, const
> > > > BlockDevOps *ops,
> > > > blk->dev_opaque = opaque;
> > > >
> > > > /* Are we currently quiesced? Should we enforce this right now? */
> > > > -if (blk->quiesce_counter && ops->drained_begin) {
> > > > +if (blk->quiesce_counter && ops && ops->drained_begin) {
> > > > ops->drained_begin(opaque);
> > > > }
> > > > }
> > >
> > > John: You added this code in f4d9cc88ee6. Does blk_set_dev_ops() need to
> > > call ->drained_end() when ops is set to NULL?
> > >
> > > Stefan
> >
> > I'm not sure I trust my memory from five years ago.
> >
> > From what I recall, the problem was that block jobs weren't getting
> > drained/paused when the backend was getting quiesced -- we wanted to
> > be sure that a blockjob wasn't continuing to run and submit requests
> > against a backend we wanted to have on ice during a sensitive
> > operation. This conditional stanza here is meant to check if the node
> > we're already attached to is *already quiesced* and we missed the
> > signal (so-to-speak), so we replay the drained_begin() request right
> > there.
> >
> > i.e. there was some case where blockjobs were getting added to an
> > already quiesced node, and this code here post-hoc relays that drain
> > request to the blockjob. This gets used in
> > 600ac6a0ef5c06418446ef2f37407bddcc51b21c to pause/unpause jobs.
> > Original thread is here:
> > https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg03416.html
> >
> > Now, I'm not sure why you want to set ops to NULL here. If we're in a
> > drained section, that sounds like it might be potentially bad because
> > we lose track of the operation to end the drained section. If your
> > intent is to destroy the thing that we'd need to call drained_end on,
> > I guess it doesn't matter -- provided you've cleaned up the target
> > object correctly. Just calling drained_end() pre-emptively seems like
> > it might be bad, what if it unpauses something you're in the middle of
> > trying to delete?
> >
> > I might need slightly more context to know what you're hoping to
> > accomplish, but I hope this info helps contextualize this code
> > somewhat.
>
> Setting to NULL in this patch is a subset of blk_detach_dev(), which
> gets called when a storage controller is hot unplugged.
>
> In this patch series there is no DeviceState because a VDUSE export is
> not a device. The VDUSE code calls blk_set_dev_ops() to
> register/unregister callbacks (e.g. ->resize_cb()).
>
> The reason I asked about ->drained_end() is for symmetry. If the
> device's ->drained_begin() callback changed state or allocated resources
> then they may need to be freed/reset. On the other hand, the
> blk_set_dev_ops(blk, NULL, NULL) call should be made by the dev_ops
> owner so they can clean up without a ->drained_end() call.
OK, got it... Hm, we don't actually use these for BlockJobs anymore.
It looks like the only user of these callbacks now is the NBD driver.
ad90febaf22d95e49fb6821bfb3ebd05b4919417 followed not long after my
initial patch and removed my intended user. I tried just removing the
fields, but the build chokes on NBD.
It looks like these usages are pretty modern, Sergio added them in
fd6afc50 (2021-06-02). So, I guess we do actually still use these
hooks. (After a period of maybe not using them for 4 years? Wow.)
I'm not clear on what we *want* to happen here, though. It doesn't
sound like NBD is the anticipated use case, so maybe just make the
removal fail if the drained section is active and callbacks are
defined? That's the safe thing to do, probably.
--js
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Tue, Mar 15, 2022 at 06:27:57PM +0100, Paolo Bonzini wrote: > On 3/15/22 10:32, Daniel P. Berrangé wrote: > > > > So NetBSD is our biggest constraint on requiring GCC 10 > > > > > > Do we care about the BSDs since they have newer compilers (including > > > gcc10) > > > available in pkgsrc? If you go by the base system, then RHEL8 has 8.5.0 > > > and > > > newer version are only available with packages such as gcc-toolset-10 and > > > gcc-toolset-11. > > > > I mention NetBSD because we had an explicit request to support > > 7.4 GCC from there, as it was the current system compiler. > > Thanks, do you have a reference? I would like to understand the reason for > that (adding Werner too). It was commit 3830df5f83b9b52d9496763ce1a50afb9231c998 > For RHEL8, considering that the switch would be several months away anyway, > I don't think it would be an issue to require AppStream toolset packages. > It's unlikely that RHEL8 would rebase QEMU in 2023 and beyond. It isn't so much RHEL8 rebases I'm thinking of wrt the policy, but rather our corporate contributors who can be constrained by annoying corporate policies to only use RHEL-8 for their work. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Summary on new backup interfaces in QEMU
Hi all!
Here I want to summarize new interfaces and use cases for backup in QEMU.
TODO for me: convert this into good rst documentation in docs/.
OK, let's begin.
First, note that drive-backup qmp command is deprecated.
Next, some terminology:
push backup: the whole process is inside QEMU process, also may be called "internal
backup"
pull backup: QEMU only exports a kind of snapshot (for example by NBD), and third party
software reads this export and stores it somehow, also called "external backup"
copy-before-write operations: We usually do backup of active disk, guest is
running and may write to the disk during the process of backup. When guest
wants to rewrite data region which is not backed up yet, we must stop this
guest write, and copy original data to somewhere before continuing guest write.
That's a copy-before-write operation.
image-fleecing: the technique that allows to export a "snapshotted" state of the active disk with help of
copy-before-write operations. We create a temporary image - target for copy-before-write operations, and provide an
interface to the user to read the "snapshotted" state. And for read, we do read from temporary image the data
which is already changed in original active disk, and we read unchanged data directly from active disk. The temporary
image itself is also called "reverse delta" or "reversed delta".
== Simple push backup ==
Just use blockdev-backup, nothing new here. I just note some technical details,
that are relatively new:
1. First, backup job inserts copy-before-write filter above source disk, to do
copy-before-write operation.
2. Created copy-before-write filter shares internal block-copy state with
backup job, so they work in collaboration, to not copy same things twice.
== Full pull backup ==
Assume, we are going to do incremental backup in future, so we also need to
create a dirty bitmap, to track dirtiness of active disk since full backup.
1. Create empty temporary image for fleecing. It must be of the same size as
active disk. It's not necessary to be qcow2, and if it's a qcow2, you shouldn't
make the original active disk a backing file for the new temporary qcow2 image
(it was necessary in old fleecing scheme).
Example:
qemu-img create -f qcow2 temp.qcow2 64G
2. Initialize fleecing scheme and create dirty bitmap for future incremental
backup.
Assume, disk0 is an active disk, attached to qdev-id sda, to be backed up.
qmp: transaction [
block-dirty-bitmap-add {node: disk0, name: bitmap0, persistent: true}
blockdev-add* {node-name: tmp-protocol, driver: file, filename: temp.qcow2}
blockdev-add {node-name: tmp, driver: qcow2, file: tmp-protocol}
blockdev-add {node-name: cbw, driver: copy-before-write, file: disk0,
target: tmp}
blockdev-replace** {parent-type: qdev, qdev-id: sda, new-child: cbw}
blockdev-add {node-name: acc, driver: snapshot-access, file: cbw}
]
qmp: nbd-server-start {...}
qmp: nbd-server-add {device: acc, ...}
This way we create the following block-graph:
[guest] [NBD export]
||
| root | root
v file v
[copy-before-write]<--[snapshot-access]
| |
| file | target
v v
[active-disk] [temp.qcow2]
* "[PATCH 0/2] blockdev-add transaction" series needed for this
** "[PATCH v3 00/11] blockdev-replace" series needed for this
Note additional useful options for copy-before-write filter:
"[PATCH 0/3] block: copy-before-write: on-cbw-error behavior" provides
possibility of option on-cbw-error=break-snapshot, which means that on failure of CBW
operation we will not break guest write, but instead all further reads by NBD client will
fail, which formally means: break the backup process, not guest write.
"[PATCH 0/4] block: copy-before-write: cbw-timeout" provides an option
cbw-timeout, to set a timeout for CBW operations. That's very useful to avoid guest stuck.
3. Now third party backup tool can read data from NBD export
NBD_CMD_TRIM (discard) operation is supported on the export, it has the
following effects:
1. discard this data from temp image, if it is stored here
2. avoid further copy-before-write operation (guest is free to rewrite
corresponding data with no extra latency)
3. all further read requests from discarded areas by NBD client will fail
So, NBD client may discard regions that are already backed up to avoid extra
latency for guest writes and to free disk space on the host.
Possible TODO here is to implement NBD protocol extension, that allows to READ
& DISCARD in command. In this case we avoid extra command in the wire, but lose
possibility of retrying the READ operation if it failed.
4. After backup is complete, we should destroy the fleecing scheme:
qmp: nbd-server-stop
qmp: blockdev-del {node-name: acc}
qmp: blockdev-replace {parent-type: qdev, qdev-id: sda, new-child: disk0}
qmp: blockdev-del {node-name: cbw}
qmp: bl
Re: [PATCH v2] block-qdict: Fix -Werror=maybe-uninitialized build failure
Hi, Philippe.
On Monday, March 14, 2022 10:47:11 AM -03 Philippe Mathieu-Daudé wrote:
> On 11/3/22 23:16, Murilo Opsfelder Araujo wrote:
> > Building QEMU on Fedora 37 (Rawhide Prerelease) ppc64le failed with the
> > following error:
> >
> > $ ../configure --prefix=/usr/local/qemu-disabletcg
> > --target-list=ppc-softmmu,ppc64-softmmu --disable-tcg --disable-linux-user
> > ...
> > $ make -j$(nproc)
> > ...
> > FAILED: libqemuutil.a.p/qobject_block-qdict.c.o
>
> This part >>>
>
> > cc -m64 -mlittle-endian -Ilibqemuutil.a.p -I. -I..
> > -Isubprojects/libvhost-user -I../subprojects/libvhost-user -Iqapi -Itrace
> > -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
> > -I/usr/include/sysprof-4 -I/usr/include/lib
> > mount -I/usr/include/blkid -I/usr/include/gio-unix-2.0
> > -I/usr/include/p11-kit-1 -I/usr/include/pixman-1 -fdiagnostics-color=auto
> > -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem
> > /root/qemu/linux-headers -isystem linux-headers -iquote
> > . -iquote /root/qemu -iquote /root/qemu/include -iquote
> > /root/qemu/disas/libvixl -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
> > -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> > -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite
> > -strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
> > -Wold-style-declaration -Wold-style-definition -Wtype-limits
> > -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
> > -Wempty-body -Wnested-externs -Wendif-label
> > s -Wexpansion-to-defined -Wimplicit-fallthrough=2
> > -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi
> > -fstack-protector-strong -fPIE -MD -MQ
> > libqemuutil.a.p/qobject_block-qdict.c.o -MF
> > libqemuutil.a.p/qobject_block-qdict.c.o.d -
> > o libqemuutil.a.p/qobject_block-qdict.c.o -c ../qobject/block-qdict.c
>
> <<< is noise (doesn't provide any value) and could be stripped.
Is this something the committer/maintainer could edit when applying the commit
or do you need I resend the v3?
Cheers!
>
> > In file included from /root/qemu/include/qapi/qmp/qdict.h:16,
> > from /root/qemu/include/block/qdict.h:13,
> > from ../qobject/block-qdict.c:11:
> > /root/qemu/include/qapi/qmp/qobject.h: In function
> > âqdict_array_splitâ:
> > /root/qemu/include/qapi/qmp/qobject.h:49:17: error: âsubqdictâ may
> > be used uninitialized [-Werror=maybe-uninitialized]
> > 49 | typeof(obj) _obj = (obj);
> > \
> >| ^~~~
> > ../qobject/block-qdict.c:227:16: note: âsubqdictâ declared here
> >227 | QDict *subqdict;
> >|^~~~
> > cc1: all warnings being treated as errors
> >
> > Fix build failure by expanding the ternary operation.
> > Tested with `make check-unit` (the check-block-qdict test passed).
> >
> > Signed-off-by: Murilo Opsfelder Araujo
> > Cc: Kevin Wolf
> > Cc: Hanna Reitz
> > Cc: Markus Armbruster
> > ---
> > v1: https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03224.html
> >
> > qobject/block-qdict.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> > index 1487cc5dd8..4a83bda2c3 100644
> > --- a/qobject/block-qdict.c
> > +++ b/qobject/block-qdict.c
> > @@ -251,12 +251,12 @@ void qdict_array_split(QDict *src, QList **dst)
> > if (is_subqdict) {
> > qdict_extract_subqdict(src, &subqdict, prefix);
> > assert(qdict_size(subqdict) > 0);
> > +qlist_append_obj(*dst, QOBJECT(subqdict));
> > } else {
> > qobject_ref(subqobj);
> > qdict_del(src, indexstr);
> > +qlist_append_obj(*dst, subqobj);
> > }
> > -
> > -qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
> > }
> > }
> >
>
>
>
>
--
Murilo
Re: [PATCH experiment 00/16] C++20 coroutine backend
On 3/15/22 17:15, Daniel P. Berrangé wrote: Bear with me as I suggest something potentially/probably silly given my limited knowledge of C++ coroutines. Given a function I know about: void coroutine_fn qio_channel_yield(QIOChannel *ioc, GIOCondition condition); IIUC, you previously indicated that the header file declaration, the implementation and any callers of this would need to be in C++ source files. The caller is what I'm most curious about, because I feel that is where the big ripple effects come into play that cause large parts of QEMU to become C++ code. [...] I presume there is something special about the CoroutineFn prototype preventing that from working as needed, thus requiring the caller to be compiled as C++ ? IIUC compiling as C++ though is not neccessarily the same as using C++ linkage. Yes, the CoroutineFn function must either be passed to qemu_coroutine_create() or called as "co_await f()". If you call it as "f()" it does nothing except leak the memory needed by its stack frame, so that only leaves passing the function to qemu_coroutine_create(). I suppose you could do some games with typedefs, like #ifdef __cplusplus typedef CoroutineFn VoidCoroutine #else typedef struct VoidCoroutine VoidCoroutine; #endif to be able to declare a function that returns CoroutineFn but I'm not sure of the advantage. So I'm assuming the caller as C++ requirement is not recursive, otherwise it would immediately mean all of QEMU needs to be C++. Right, qemu_coroutine_create() must be called from C++ but the caller of qemu_coroutine_create() can be extern "C". In the particular case of the block layer, callers of qemu_coroutine_create() include callback-based functions such as bdrv_aio_readv(), and synchronous functions such as bdrv_flush(). Both of these can be called from C. IOW, can we get it such that the C++ bit is just a thin shim "C -> C++ wrapper -> C++ CoroutineFn -> C", enabling all the C++ bits to be well encapsulated and thus prevent arbitrary usage of C++ features leaking all across the codebase ? No, unfortunately not. But in particular, even though the block layer would be C++, device models that use it would not. Paolo
Re: [PATCH experiment 00/16] C++20 coroutine backend
On 3/15/22 15:24, Peter Maydell wrote: On Tue, 15 Mar 2022 at 14:09, Stefan Hajnoczi wrote: Also, once C++ is available people will start submitting C++ patches simply because they are more comfortable with C++ (especially one-time/infrequent contributors). This to my mind is the major argument against using C++ for coroutines... I agree on the need for a policy, but _what_ C++ are they going to be contributing that we should be scared of? We're talking about: * major features contributed by one-time/infrequent participants (which is already a once-in-a-year thing or so, at least for me) * ... in an area where there are no examples of using C++ in the tree (or presumably the maintainer would be comfortable reviewing it) * ... but yet C++ offer killer features (right now there's only C++ coroutines and fpu/) * ... and where the one-time contributor has put enough investment in using these killer C++ features, that telling them to remove the features would amount to a rewrite. That does not seem to be a common situation, and not even a problematic one if it were to happen. Paolo
Re: [PATCH experiment 00/16] C++20 coroutine backend
On 3/15/22 10:32, Daniel P. Berrangé wrote: So NetBSD is our biggest constraint on requiring GCC 10 Do we care about the BSDs since they have newer compilers (including gcc10) available in pkgsrc? If you go by the base system, then RHEL8 has 8.5.0 and newer version are only available with packages such as gcc-toolset-10 and gcc-toolset-11. I mention NetBSD because we had an explicit request to support 7.4 GCC from there, as it was the current system compiler. Thanks, do you have a reference? I would like to understand the reason for that (adding Werner too). For RHEL8, considering that the switch would be several months away anyway, I don't think it would be an issue to require AppStream toolset packages. It's unlikely that RHEL8 would rebase QEMU in 2023 and beyond. Paolo
When and how to use C++ (was Re: [PATCH experiment 00/16] C++20 coroutine backend)
On 3/15/22 15:50, Kevin Wolf wrote:
I'm not sure what the C++ lock guards offer that our current lock guards
don't? Passing down lock guards makes sense to me, but why can't you do
that with QemuLockable?
Passing a QemuLockable alone doesn't ensure that the lock has been
taken. I guess we could build a QemuLockGuard on top that ensures that.
(Hm, or can the C++ version somehow check at
compile time that it's the _right_ lock that is held rather than just
any lock? It didn't look like it at the first sight.)
It's not related to lock guards but clang++ has thread-safety analysis
that checks for the right lock. It is in theory there for C as well but
it's mostly a joke. Ironically, the only good use that I've seen of it
is Marc-André's never committed work to check coroutine_fn markers at
compile-time[1].
But I do see the benefit of a compiler checked CoroutineFn<> return type
compared to the coroutine_fn markers we have today. On the other hand...
Also, once C++ is available people will start submitting C++ patches
simply because they are more comfortable with C++ (especially
one-time/infrequent contributors).
...using C++ in coroutine code means that all of the block layer would
suddenly become C++ and would be most affected by this effect. I'm not
sure if that's something I would like to see, at least until there is a
clear tree-wide policy (that preferably limits it to a subset that I
understand).
You are the maintainer of the block layer, so you would have the biggest
impact and obviously have input in such a policy. I obviously don't
have a policy ready[2], but I do have some goals:
1) whenever C++ is used to build an API for use in C++ source files:
Those source files and their debugging experience should look
like C, and not any more "foreign" than the "QEMU C" dialect
that we already have.
Rationale: we might have funky code in headers, but
"QEMU_WITH_LOCK_GUARD(x) { }" is understandable; the same would
hold for "CoroutineFn", "co_await fn();" or if hwaddr
internally used operator overloading.
2) whenever C and C++ provide two different implementations of an API,
for use in C and C++ source files respectively:
If the same API is implemented in different ways for C and C++,
the semantics should be equivalent, and covered by both
C and C++ testcases.
Rationale: this should be a rare occasion, but there are
features such as _Generic that are C only, so it should be
catered for; QemuLockable came out already in this proof of
concept. The implementation is completely different for C
and C++, but it works the same (as proved to some extent
by testcases).
Looking again at the hwaddr example, it would be okay to forbid
"hwaddr - hwaddr" subtraction in C++ code. But it would not be
okay to have it return a signed integer in C++, while it
returns an unsigned integer in C. Could there be a C++-only
method "hwaddr.diff(hwaddr)" that does return a signed integer?
That would have to be discussed, right now my opinion is
"probably not" but see the next point too.
3) whenever C++ features (templates, classes, etc.) are used in .cc files:
The features should have a clear benefit over equivalent
C-only code (e.g. using the preprocessor instead of templates)
and if applicable they should publish a C API[3] that can be
consumed outside a particular subsystem.
If in doubt, err on the side of moderation, i.e. less C++ and
fewer C++ features.
Example: I can imagine that in some cases the semantic
replacement abilities provided by templates will for example
improve compile-time checking and reduce code duplication
compared to gcc. However, QEMU already has a wide range of
data structures and auxiliary APIs for C, and those should be
used for C++ code to avoid splitting the code base.
In turn, this should limit the usage of other C++ features.
For example, constructors (as opposed to just being able to
zero-initialize memory) can be more of a pain than a benefit
if you can't use "new", "delete" or "std::vector<>". Likewise,
it's unlikely that we'll have reasons to use anonymous
namespaces instead of "static", because that's mostly useful
for classes defined in .cc files and their member functions[5].
And VMState will prevent the usage of "private:", thus lowering
the benefit from methods.
Would this kind of formulation be enough? Maybe, maybe not, but either
way I don't really believe in prohibiting features by policy. The QEMU
community and especially its maintainers won't change overnight if C++
is allowed in the codebase; but they should be allowed to evolve their
opinions as their experience grows. If some day t
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Markus Armbruster writes:
> Philippe Mathieu-Daudé writes:
>
>> On 15/3/22 14:59, Markus Armbruster wrote:
>>> Alex Bennée writes:
>>>
Markus Armbruster writes:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> diff --git a/semihosting/config.c b/semihosting/config.c
> index 137171b717..6d48ec9566 100644
> --- a/semihosting/config.c
> +++ b/semihosting/config.c
> @@ -98,7 +98,7 @@ static int add_semihosting_arg(void *opaque,
> if (strcmp(name, "arg") == 0) {
> s->argc++;
> /* one extra element as g_strjoinv() expects NULL-terminated
> array */
> -s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
> +s->argv = g_renew(void *, s->argv, s->argc + 1);
This did indeed break CI because s->argv is an array of *char:
../semihosting/config.c:101:17: error: assignment to ‘const char **’ from
incompatible pointer type ‘void **’ [-Werror=incompatible-pointer-types]
101 | s->argv = g_renew(void *, s->argv, s->argc + 1);
| ^
cc1: all warnings being treated as errors
So it did the job of type checking but failed to build ;-)
>>>
>>> You found a hole in my compile testing, thanks!
>>>
>>> I got confused about the configuration of my build trees. Catching such
>>> mistakes is what CI is for :)
>>
>> FYI Alex fixed this here:
>> https://lore.kernel.org/qemu-devel/20220315121251.2280317-8-alex.ben...@linaro.org/
>>
>> So your series could go on top (modulo the Coverity change).
>
> I dropped this hunk in v2.
>
> Whether my v2 or Alex's series goes in first doesn't matter.
That's great. Thanks for finding the ugliness in the first place ;-)
--
Alex Bennée
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Mon, Mar 14, 2022 at 10:31:47AM +0100, Paolo Bonzini wrote:
> However, there are no ramifications to actual coroutine code, except
> for the template syntax "CoroutineFn" for the function and
> the mandatory co_await/co_return keywords... both of which are an
> improvement, really: the fact that a single function cannot run either
> inside or outside coroutines is checked by the compiler now, because
> qemu_coroutine_create accepts a function that returns CoroutineFn.
> Therefore I had to disable some more code in util/ and qapi/ that used
> qemu_in_coroutine() or coroutine_fn.
Bear with me as I suggest something potentially/probably silly
given my limited knowledge of C++ coroutines.
Given a function I know about:
void coroutine_fn qio_channel_yield(QIOChannel *ioc,
GIOCondition condition);
IIUC, you previously indicated that the header file declaration,
the implementation and any callers of this would need to be in
C++ source files.
The caller is what I'm most curious about, because I feel that
is where the big ripple effects come into play that cause large
parts of QEMU to become C++ code.
In general it is possible to call C++ functions from C.
I presume there is something special about the CoroutineFn
prototype preventing that from working as needed, thus requiring
the caller to be compiled as C++ ? IIUC compiling as C++ though
is not neccessarily the same as using C++ linkage.
So I'm assuming the caller as C++ requirement is not recursive,
otherwise it would immediately mean all of QEMU needs to be C++.
This leads me to wonder if we can workaround this problem with
a wrapper function. eg in a io/channel.hpp file can be declare
something like:
CoroutineFn qio_channel_yield_impl(QIOChannel *ioc,
GIOCondition condition);
extern "C" {
static inline void qio_chanel_yield(QIOChannel *ioc,
GIOCondition condition) {
qio_channel_yield_impl(ioc, condition)
}
}
With this qio_channel_yield_impl and qio_channel_yield are both
compiled as C++, but qio_channel_yield is exposed with C linkage
semantics. Thus enabling callers of qio_channel_yield can carry
on being compiled as C, since the invokation of the CoroutineFn
is in the inline C++ function ?
This would mean an extra function call, but perhaps this gets
optimized away, espeically with LTO, such that it doesn't impact
performance negatively ?
The impl of qio_channel_yield_impl() could also call from C++
back to C functions as much as possible.
IOW, can we get it such that the C++ bit is just a thin shim
"C -> C++ wrapper -> C++ CoroutineFn -> C", enabling all the
C++ bits to be well encapsulated and thus prevent arbitrary
usage of C++ features leaking all across the codebase ?
With Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Tue, Mar 15, 2022 at 03:50:26PM +0100, Kevin Wolf wrote: > Am 15.03.2022 um 15:05 hat Stefan Hajnoczi geschrieben: > > On Mon, Mar 14, 2022 at 05:21:22PM +0100, Paolo Bonzini wrote: > > > On 3/14/22 15:07, Stefan Hajnoczi wrote: > > > > If we can reach a consensus about C++ language usage in QEMU then I'm in > > > > favor of using C++ coroutines. It's probably not realistic to think we > > > > can limit C++ language usage to just coroutines forever. Someone finds > > > > another C++ feature they absolutely need and over time the codebase > > > > becomes C++ - with both its advantages and disadvantages. > > > > > > > > [...] although you can write C in C++, it's not idiomatic modern C++. > > > > The language lends itself to a different style of programming that > > > > some will embrace while others will not. > > > > > > Yes, this is an important aspect to discuss. I think coroutines provide a > > > good blueprint for how QEMU might use C++. > > > > > > I totally agree that, if we go this way, the genie is out of the bottle > > > and > > > other uses of C++ will pop up with 100% probability. But the important > > > thing to note is that our dialect of C is already not standard C, and that > > > some of our or GLib's "innovations" are actually based on experience with > > > C++. We can keep on writing "QEMU's C" if we think of C++ as a > > > supercharged > > > way of writing these quality-of-life improvements that we already write. > > > In > > > some sense coroutines are an extreme example of this idea. > > > > > > In fact, a C API would have to remain unless all source files are changed > > > to > > > C++, so QEMU would remain mostly a C project with C idioms, but that > > > doesn't > > > prevent _abstracting_ the use of C++ features (written in modern, > > > idiomatic > > > C++) behind an API that C programmers have no problems learning. Again, > > > coroutines are an example of this of keeping the familiar > > > create/enter/yield > > > API and hiding the "magic" of C++ coroutines (and when they don't, that > > > had > > > better be an improvement). > > > > > > In the end, C++ is a tool that you can use if it leads to better code. For > > > example, I don't see much use of C++ for devices for example, and the > > > storage devices in particular do not even need coroutines because they use > > > the callback-based interface. But perhaps someone will try to use > > > templates > > > to replace repeated inclusion (which is common in hw/display) and others > > > will follow suit. Or perhaps not. > > > > > > One example that was brought up on IRC is type-safe operations on things > > > such as hwaddr (i.e. hwaddr+int is allowed but hwaddr-hwaddr gives back an > > > int64_t and might even check for overflow). These would be opt in (you > > > get > > > them just by changing a file from .c to .cc), but the actual C++ code > > > would > > > still look very much like C code that uses hwaddr with no type checking. > > > All the operator overloading gunk would be in include/. > > > > > > A different topic is what would happen if all of QEMU could be compiled as > > > C++, and could inform our selection of C++ idioms even long before we get > > > there. For example, I'm fine with GLib and our type-safe intrusive lists, > > > so I don't have much interest in STL containers and I would prefer _not_ > > > to > > > use STL containers even in .cc files[1]. However, perhaps QEMU's > > > home-grown > > > lock guard might be replaced by something that uses C++ destructors > > > instead > > > of g_autoptr, so perhaps we should consider using std::lock_guard<>, or > > > something like that, instead of QEMU_LOCK_GUARD. It may be interesting to > > > pass down lock_guards as arguments to enforce "this lock is taken" > > > invariants. > > > > > > But really, coroutines would be enough work so my dish would be full for > > > some time and I wouldn't really have time to look at any of this. :) > > > > I think it will be necessary to compile QEMU with a C++ compiler quite > > soon. It is possible to provide C APIs like in the case of coroutines, > > but sometimes C++ features need to be exposed to the caller (like the > > lock guards you mentioned). > > I'm not sure what the C++ lock guards offer that our current lock guards > don't? Passing down lock guards makes sense to me, but why can't you do > that with QemuLockable? (Hm, or can the C++ version somehow check at > compile time that it's the _right_ lock that is held rather than just > any lock? It didn't look like it at the first sight.) > > But I do see the benefit of a compiler checked CoroutineFn<> return type > compared to the coroutine_fn markers we have today. On the other hand... > > > Also, once C++ is available people will start submitting C++ patches > > simply because they are more comfortable with C++ (especially > > one-time/infrequent contributors). > > ...using C++ in coroutine code means that all of the block layer wou
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Tue, Mar 15, 2022 at 03:50:26PM +0100, Kevin Wolf wrote: > Am 15.03.2022 um 15:05 hat Stefan Hajnoczi geschrieben: > > On Mon, Mar 14, 2022 at 05:21:22PM +0100, Paolo Bonzini wrote: > > > On 3/14/22 15:07, Stefan Hajnoczi wrote: > > > > If we can reach a consensus about C++ language usage in QEMU then I'm in > > > > favor of using C++ coroutines. It's probably not realistic to think we > > > > can limit C++ language usage to just coroutines forever. Someone finds > > > > another C++ feature they absolutely need and over time the codebase > > > > becomes C++ - with both its advantages and disadvantages. > > > > > > > > [...] although you can write C in C++, it's not idiomatic modern C++. > > > > The language lends itself to a different style of programming that > > > > some will embrace while others will not. > > > > > > Yes, this is an important aspect to discuss. I think coroutines provide a > > > good blueprint for how QEMU might use C++. > > > > > > I totally agree that, if we go this way, the genie is out of the bottle > > > and > > > other uses of C++ will pop up with 100% probability. But the important > > > thing to note is that our dialect of C is already not standard C, and that > > > some of our or GLib's "innovations" are actually based on experience with > > > C++. We can keep on writing "QEMU's C" if we think of C++ as a > > > supercharged > > > way of writing these quality-of-life improvements that we already write. > > > In > > > some sense coroutines are an extreme example of this idea. > > > > > > In fact, a C API would have to remain unless all source files are changed > > > to > > > C++, so QEMU would remain mostly a C project with C idioms, but that > > > doesn't > > > prevent _abstracting_ the use of C++ features (written in modern, > > > idiomatic > > > C++) behind an API that C programmers have no problems learning. Again, > > > coroutines are an example of this of keeping the familiar > > > create/enter/yield > > > API and hiding the "magic" of C++ coroutines (and when they don't, that > > > had > > > better be an improvement). > > > > > > In the end, C++ is a tool that you can use if it leads to better code. For > > > example, I don't see much use of C++ for devices for example, and the > > > storage devices in particular do not even need coroutines because they use > > > the callback-based interface. But perhaps someone will try to use > > > templates > > > to replace repeated inclusion (which is common in hw/display) and others > > > will follow suit. Or perhaps not. > > > > > > One example that was brought up on IRC is type-safe operations on things > > > such as hwaddr (i.e. hwaddr+int is allowed but hwaddr-hwaddr gives back an > > > int64_t and might even check for overflow). These would be opt in (you > > > get > > > them just by changing a file from .c to .cc), but the actual C++ code > > > would > > > still look very much like C code that uses hwaddr with no type checking. > > > All the operator overloading gunk would be in include/. > > > > > > A different topic is what would happen if all of QEMU could be compiled as > > > C++, and could inform our selection of C++ idioms even long before we get > > > there. For example, I'm fine with GLib and our type-safe intrusive lists, > > > so I don't have much interest in STL containers and I would prefer _not_ > > > to > > > use STL containers even in .cc files[1]. However, perhaps QEMU's > > > home-grown > > > lock guard might be replaced by something that uses C++ destructors > > > instead > > > of g_autoptr, so perhaps we should consider using std::lock_guard<>, or > > > something like that, instead of QEMU_LOCK_GUARD. It may be interesting to > > > pass down lock_guards as arguments to enforce "this lock is taken" > > > invariants. > > > > > > But really, coroutines would be enough work so my dish would be full for > > > some time and I wouldn't really have time to look at any of this. :) > > > > I think it will be necessary to compile QEMU with a C++ compiler quite > > soon. It is possible to provide C APIs like in the case of coroutines, > > but sometimes C++ features need to be exposed to the caller (like the > > lock guards you mentioned). > > I'm not sure what the C++ lock guards offer that our current lock guards > don't? Passing down lock guards makes sense to me, but why can't you do > that with QemuLockable? (Hm, or can the C++ version somehow check at > compile time that it's the _right_ lock that is held rather than just > any lock? It didn't look like it at the first sight.) > > But I do see the benefit of a compiler checked CoroutineFn<> return type > compared to the coroutine_fn markers we have today. On the other hand... Sorry, I made a mistake: the C++ coroutines implementation does not hide everything behind a C API. Coroutine functions need to be defined in C++ source units. Stefan signature.asc Description: PGP signature
Re: [PATCH experiment 00/16] C++20 coroutine backend
Am 15.03.2022 um 15:05 hat Stefan Hajnoczi geschrieben: > On Mon, Mar 14, 2022 at 05:21:22PM +0100, Paolo Bonzini wrote: > > On 3/14/22 15:07, Stefan Hajnoczi wrote: > > > If we can reach a consensus about C++ language usage in QEMU then I'm in > > > favor of using C++ coroutines. It's probably not realistic to think we > > > can limit C++ language usage to just coroutines forever. Someone finds > > > another C++ feature they absolutely need and over time the codebase > > > becomes C++ - with both its advantages and disadvantages. > > > > > > [...] although you can write C in C++, it's not idiomatic modern C++. > > > The language lends itself to a different style of programming that > > > some will embrace while others will not. > > > > Yes, this is an important aspect to discuss. I think coroutines provide a > > good blueprint for how QEMU might use C++. > > > > I totally agree that, if we go this way, the genie is out of the bottle and > > other uses of C++ will pop up with 100% probability. But the important > > thing to note is that our dialect of C is already not standard C, and that > > some of our or GLib's "innovations" are actually based on experience with > > C++. We can keep on writing "QEMU's C" if we think of C++ as a supercharged > > way of writing these quality-of-life improvements that we already write. In > > some sense coroutines are an extreme example of this idea. > > > > In fact, a C API would have to remain unless all source files are changed to > > C++, so QEMU would remain mostly a C project with C idioms, but that doesn't > > prevent _abstracting_ the use of C++ features (written in modern, idiomatic > > C++) behind an API that C programmers have no problems learning. Again, > > coroutines are an example of this of keeping the familiar create/enter/yield > > API and hiding the "magic" of C++ coroutines (and when they don't, that had > > better be an improvement). > > > > In the end, C++ is a tool that you can use if it leads to better code. For > > example, I don't see much use of C++ for devices for example, and the > > storage devices in particular do not even need coroutines because they use > > the callback-based interface. But perhaps someone will try to use templates > > to replace repeated inclusion (which is common in hw/display) and others > > will follow suit. Or perhaps not. > > > > One example that was brought up on IRC is type-safe operations on things > > such as hwaddr (i.e. hwaddr+int is allowed but hwaddr-hwaddr gives back an > > int64_t and might even check for overflow). These would be opt in (you get > > them just by changing a file from .c to .cc), but the actual C++ code would > > still look very much like C code that uses hwaddr with no type checking. > > All the operator overloading gunk would be in include/. > > > > A different topic is what would happen if all of QEMU could be compiled as > > C++, and could inform our selection of C++ idioms even long before we get > > there. For example, I'm fine with GLib and our type-safe intrusive lists, > > so I don't have much interest in STL containers and I would prefer _not_ to > > use STL containers even in .cc files[1]. However, perhaps QEMU's home-grown > > lock guard might be replaced by something that uses C++ destructors instead > > of g_autoptr, so perhaps we should consider using std::lock_guard<>, or > > something like that, instead of QEMU_LOCK_GUARD. It may be interesting to > > pass down lock_guards as arguments to enforce "this lock is taken" > > invariants. > > > > But really, coroutines would be enough work so my dish would be full for > > some time and I wouldn't really have time to look at any of this. :) > > I think it will be necessary to compile QEMU with a C++ compiler quite > soon. It is possible to provide C APIs like in the case of coroutines, > but sometimes C++ features need to be exposed to the caller (like the > lock guards you mentioned). I'm not sure what the C++ lock guards offer that our current lock guards don't? Passing down lock guards makes sense to me, but why can't you do that with QemuLockable? (Hm, or can the C++ version somehow check at compile time that it's the _right_ lock that is held rather than just any lock? It didn't look like it at the first sight.) But I do see the benefit of a compiler checked CoroutineFn<> return type compared to the coroutine_fn markers we have today. On the other hand... > Also, once C++ is available people will start submitting C++ patches > simply because they are more comfortable with C++ (especially > one-time/infrequent contributors). ...using C++ in coroutine code means that all of the block layer would suddenly become C++ and would be most affected by this effect. I'm not sure if that's something I would like to see, at least until there is a clear tree-wide policy (that preferably limits it to a subset that I understand). Kevin signature.asc Description: PGP signature
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Philippe Mathieu-Daudé writes:
> On 15/3/22 14:59, Markus Armbruster wrote:
>> Alex Bennée writes:
>>
>>> Markus Armbruster writes:
>>>
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
for two reasons. One, it catches multiplication overflowing size_t.
Two, it returns T * rather than void *, which lets the compiler catch
more type errors.
>>>
diff --git a/semihosting/config.c b/semihosting/config.c
index 137171b717..6d48ec9566 100644
--- a/semihosting/config.c
+++ b/semihosting/config.c
@@ -98,7 +98,7 @@ static int add_semihosting_arg(void *opaque,
if (strcmp(name, "arg") == 0) {
s->argc++;
/* one extra element as g_strjoinv() expects NULL-terminated
array */
-s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
+s->argv = g_renew(void *, s->argv, s->argc + 1);
>>>
>>> This did indeed break CI because s->argv is an array of *char:
>>>
>>> ../semihosting/config.c:101:17: error: assignment to ‘const char **’ from
>>> incompatible pointer type ‘void **’ [-Werror=incompatible-pointer-types]
>>>101 | s->argv = g_renew(void *, s->argv, s->argc + 1);
>>>| ^
>>> cc1: all warnings being treated as errors
>>>
>>> So it did the job of type checking but failed to build ;-)
>>
>> You found a hole in my compile testing, thanks!
>>
>> I got confused about the configuration of my build trees. Catching such
>> mistakes is what CI is for :)
>
> FYI Alex fixed this here:
> https://lore.kernel.org/qemu-devel/20220315121251.2280317-8-alex.ben...@linaro.org/
>
> So your series could go on top (modulo the Coverity change).
I dropped this hunk in v2.
Whether my v2 or Alex's series goes in first doesn't matter.
[PATCH v2 0/3] Use g_new() & friends where that makes obvious sense
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors. This series only touches allocations with size arguments of the form sizeof(T). It's mechanical, except for a tiny fix in PATCH 2. PATCH 1 adds the Coccinelle script. PATCH 2 cleans up the virtio-9p subsystem, and fixes a harmless typing error uncovered by the cleanup. PATCH 3 cleans up everything else. I started to split it up, but splitting is a lot of decisions, and I just can't see the value. For instance, MAINTAINERS tells me to split for subsystem "virtio", patching hw/char/virtio-serial-bus.c hw/display/virtio-gpu.c hw/net/virtio-net.c hw/virtio/virtio-crypto.c hw/virtio/virtio-iommu.c hw/virtio/virtio.c But it also tells me to split for subsystem "Character devices", patching hw/char/parallel.c | 2 +- hw/char/riscv_htif.c | 2 +- hw/char/virtio-serial-bus.c | 6 +- and for subsystem "Network devices", patching hw/net/virtio-net.c and for subsystem "virtio-gpu", patching hw/display/virtio-gpu.c I guess I'd go with "virtio". Six files down, 103 to go. Thanks, but no thanks. Since the transformation is local to a function call, dropping is completely safe. We can deal with conflicts by dropping conflicting hunks, with "git-pull -s recursive -X ours". Or drop entire files with conflicts. If you want me to split off certain parts, please tell me exactly what you want split off, and I'll gladly do the splitting. I don't mind the splitting part, I do mind the *thinking* part. I backed out two changes made by the Coccinelle script: scripts/coverity-scan/model.c, because that's special, and semihosting/config.c, because it has a typing error similar to the one fixed in PATCH 2, and Alex already posted a patch for it. v2: * PATCH 3: Change to scripts/coverity-scan/model.c dropped [Eric] * PATCH 3: Change to semihosting/config.c dropped [Alex] * Commit messages tweaked Markus Armbruster (3): scripts/coccinelle: New use-g_new-etc.cocci 9pfs: Use g_new() & friends where that makes obvious sense Use g_new() & friends where that makes obvious sense scripts/coccinelle/use-g_new-etc.cocci | 75 include/qemu/timer.h | 2 +- accel/kvm/kvm-all.c | 6 +- accel/tcg/tcg-accel-ops-mttcg.c | 2 +- accel/tcg/tcg-accel-ops-rr.c | 4 +- audio/audio.c| 4 +- audio/audio_legacy.c | 6 +- audio/dsoundaudio.c | 2 +- audio/jackaudio.c| 6 +- audio/paaudio.c | 4 +- backends/cryptodev.c | 2 +- contrib/vhost-user-gpu/vhost-user-gpu.c | 2 +- cpus-common.c| 4 +- dump/dump.c | 2 +- hw/9pfs/9p-proxy.c | 2 +- hw/9pfs/9p-synth.c | 4 +- hw/9pfs/9p.c | 8 +-- hw/9pfs/codir.c | 6 +- hw/acpi/hmat.c | 2 +- hw/audio/intel-hda.c | 2 +- hw/char/parallel.c | 2 +- hw/char/riscv_htif.c | 2 +- hw/char/virtio-serial-bus.c | 6 +- hw/core/irq.c| 2 +- hw/core/reset.c | 2 +- hw/display/pxa2xx_lcd.c | 2 +- hw/display/tc6393xb.c| 2 +- hw/display/virtio-gpu.c | 4 +- hw/display/xenfb.c | 4 +- hw/dma/rc4030.c | 4 +- hw/i2c/core.c| 4 +- hw/i2c/i2c_mux_pca954x.c | 2 +- hw/i386/amd_iommu.c | 4 +- hw/i386/intel_iommu.c| 2 +- hw/i386/xen/xen-hvm.c| 10 ++-- hw/i386/xen/xen-mapcache.c | 14 ++--- hw/input/lasips2.c | 2 +- hw/input/pckbd.c | 2 +- hw/input/ps2.c | 4 +- hw/input/pxa2xx_keypad.c | 2 +- hw/input/tsc2005.c | 3 +- hw/intc/riscv_aclint.c | 6 +- hw/intc/xics.c | 2 +- hw/m68k/virt.c | 2 +- hw/mips/mipssim.c| 2 +- hw/misc/applesmc.c | 2 +- hw/misc/imx6_src.c | 2 +- hw/misc/ivshmem.c| 4 +- hw/net/virtio-net.c | 4 +- hw/nvme/ns.c | 2 +- hw/pci-host/pnv_phb3.c | 2 +- hw/pci-host/pnv_phb4.c | 2 +- hw/pci/pcie_sriov.c
[PATCH v2 2/3] 9pfs: Use g_new() & friends where that makes obvious sense
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
for two reasons. One, it catches multiplication overflowing size_t.
Two, it returns T * rather than void *, which lets the compiler catch
more type errors.
This commit only touches allocations with size arguments of the form
sizeof(T).
Initial patch created mechanically with:
$ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
--macro-file scripts/cocci-macro-file.h FILES...
This uncovers a typing error:
../hw/9pfs/9p.c: In function ‘qid_path_fullmap’:
../hw/9pfs/9p.c:855:13: error: assignment to ‘QpfEntry *’ from incompatible
pointer type ‘QppEntry *’ [-Werror=incompatible-pointer-types]
855 | val = g_new0(QppEntry, 1);
| ^
Harmless, because QppEntry is larger than QpfEntry. Manually fixed to
allocate a QpfEntry instead.
Cc: Greg Kurz
Cc: Christian Schoenebeck
Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Christian Schoenebeck
Reviewed-by: Alex Bennée
Reviewed-by: Greg Kurz
---
hw/9pfs/9p-proxy.c | 2 +-
hw/9pfs/9p-synth.c | 4 ++--
hw/9pfs/9p.c | 8
hw/9pfs/codir.c | 6 +++---
tests/qtest/virtio-9p-test.c | 4 ++--
5 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 8b4b5cf7dc..4c5e0fc217 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -1187,7 +1187,7 @@ static int proxy_parse_opts(QemuOpts *opts, FsDriverEntry
*fs, Error **errp)
static int proxy_init(FsContext *ctx, Error **errp)
{
-V9fsProxy *proxy = g_malloc(sizeof(V9fsProxy));
+V9fsProxy *proxy = g_new(V9fsProxy, 1);
int sock_id;
if (ctx->export_flags & V9FS_PROXY_SOCK_NAME) {
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index b3080e415b..d99d263985 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -49,7 +49,7 @@ static V9fsSynthNode *v9fs_add_dir_node(V9fsSynthNode
*parent, int mode,
/* Add directory type and remove write bits */
mode = ((mode & 0777) | S_IFDIR) & ~(S_IWUSR | S_IWGRP | S_IWOTH);
-node = g_malloc0(sizeof(V9fsSynthNode));
+node = g_new0(V9fsSynthNode, 1);
if (attr) {
/* We are adding .. or . entries */
node->attr = attr;
@@ -128,7 +128,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int
mode,
}
/* Add file type and remove write bits */
mode = ((mode & 0777) | S_IFREG);
-node = g_malloc0(sizeof(V9fsSynthNode));
+node = g_new0(V9fsSynthNode, 1);
node->attr = &node->actual_attr;
node->attr->inode = synth_node_count++;
node->attr->nlink = 1;
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index a6d6b3f835..8e9d4aea73 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -324,7 +324,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
return NULL;
}
}
-f = g_malloc0(sizeof(V9fsFidState));
+f = g_new0(V9fsFidState, 1);
f->fid = fid;
f->fid_type = P9_FID_NONE;
f->ref = 1;
@@ -804,7 +804,7 @@ static int qid_inode_prefix_hash_bits(V9fsPDU *pdu, dev_t
dev)
val = qht_lookup(&pdu->s->qpd_table, &lookup, hash);
if (!val) {
-val = g_malloc0(sizeof(QpdEntry));
+val = g_new0(QpdEntry, 1);
*val = lookup;
affix = affixForIndex(pdu->s->qp_affix_next);
val->prefix_bits = affix.bits;
@@ -852,7 +852,7 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat
*stbuf,
return -ENFILE;
}
-val = g_malloc0(sizeof(QppEntry));
+val = g_new0(QpfEntry, 1);
*val = lookup;
/* new unique inode and device combo */
@@ -928,7 +928,7 @@ static int qid_path_suffixmap(V9fsPDU *pdu, const struct
stat *stbuf,
return -ENFILE;
}
-val = g_malloc0(sizeof(QppEntry));
+val = g_new0(QppEntry, 1);
*val = lookup;
/* new unique inode affix and device combo */
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 75148bc985..93ba44fb75 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -141,9 +141,9 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
/* append next node to result chain */
if (!e) {
-*entries = e = g_malloc0(sizeof(V9fsDirEnt));
+*entries = e = g_new0(V9fsDirEnt, 1);
} else {
-e = e->next = g_malloc0(sizeof(V9fsDirEnt));
+e = e->next = g_new0(V9fsDirEnt, 1);
}
e->dent = qemu_dirent_dup(dent);
@@ -163,7 +163,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
break;
}
-e->st = g_malloc0(sizeof(struct stat));
+e->st = g_new0(struct stat, 1);
memcpy(e->st, &stbuf, sizeof(struct stat));
}
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 01ca076afe..e2
[PATCH v2 3/3] Use g_new() & friends where that makes obvious sense
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors. This commit only touches allocations with size arguments of the form sizeof(T). Patch created mechanically with: $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \ --macro-file scripts/cocci-macro-file.h FILES... Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cédric Le Goater Reviewed-by: Alex Bennée Acked-by: Dr. David Alan Gilbert --- include/qemu/timer.h | 2 +- accel/kvm/kvm-all.c | 6 ++-- accel/tcg/tcg-accel-ops-mttcg.c | 2 +- accel/tcg/tcg-accel-ops-rr.c | 4 +-- audio/audio.c| 4 +-- audio/audio_legacy.c | 6 ++-- audio/dsoundaudio.c | 2 +- audio/jackaudio.c| 6 ++-- audio/paaudio.c | 4 +-- backends/cryptodev.c | 2 +- contrib/vhost-user-gpu/vhost-user-gpu.c | 2 +- cpus-common.c| 4 +-- dump/dump.c | 2 +- hw/acpi/hmat.c | 2 +- hw/audio/intel-hda.c | 2 +- hw/char/parallel.c | 2 +- hw/char/riscv_htif.c | 2 +- hw/char/virtio-serial-bus.c | 6 ++-- hw/core/irq.c| 2 +- hw/core/reset.c | 2 +- hw/display/pxa2xx_lcd.c | 2 +- hw/display/tc6393xb.c| 2 +- hw/display/virtio-gpu.c | 4 +-- hw/display/xenfb.c | 4 +-- hw/dma/rc4030.c | 4 +-- hw/i2c/core.c| 4 +-- hw/i2c/i2c_mux_pca954x.c | 2 +- hw/i386/amd_iommu.c | 4 +-- hw/i386/intel_iommu.c| 2 +- hw/i386/xen/xen-hvm.c| 10 +++--- hw/i386/xen/xen-mapcache.c | 14 hw/input/lasips2.c | 2 +- hw/input/pckbd.c | 2 +- hw/input/ps2.c | 4 +-- hw/input/pxa2xx_keypad.c | 2 +- hw/input/tsc2005.c | 3 +- hw/intc/riscv_aclint.c | 6 ++-- hw/intc/xics.c | 2 +- hw/m68k/virt.c | 2 +- hw/mips/mipssim.c| 2 +- hw/misc/applesmc.c | 2 +- hw/misc/imx6_src.c | 2 +- hw/misc/ivshmem.c| 4 +-- hw/net/virtio-net.c | 4 +-- hw/nvme/ns.c | 2 +- hw/pci-host/pnv_phb3.c | 2 +- hw/pci-host/pnv_phb4.c | 2 +- hw/pci/pcie_sriov.c | 2 +- hw/ppc/e500.c| 2 +- hw/ppc/ppc.c | 8 ++--- hw/ppc/ppc405_boards.c | 4 +-- hw/ppc/ppc405_uc.c | 18 +- hw/ppc/ppc4xx_devs.c | 2 +- hw/ppc/ppc_booke.c | 4 +-- hw/ppc/spapr.c | 2 +- hw/ppc/spapr_events.c| 2 +- hw/ppc/spapr_hcall.c | 2 +- hw/ppc/spapr_numa.c | 3 +- hw/rdma/vmw/pvrdma_dev_ring.c| 2 +- hw/rdma/vmw/pvrdma_qp_ops.c | 6 ++-- hw/sh4/r2d.c | 4 +-- hw/sh4/sh7750.c | 2 +- hw/sparc/leon3.c | 2 +- hw/sparc64/sparc64.c | 4 +-- hw/timer/arm_timer.c | 2 +- hw/timer/slavio_timer.c | 2 +- hw/vfio/pci.c| 4 +-- hw/vfio/platform.c | 4 +-- hw/virtio/virtio-crypto.c| 2 +- hw/virtio/virtio-iommu.c | 2 +- hw/virtio/virtio.c | 5 ++- hw/xtensa/xtfpga.c | 2 +- linux-user/syscall.c | 2 +- migration/dirtyrate.c| 4 +-- migration/multifd-zlib.c | 4 +-- migration/ram.c | 2 +- monitor/misc.c | 2 +- monitor/qmp-cmds.c | 2 +- qga/commands-win32.c | 8 ++--- qga/commands.c | 2 +- qom/qom-qmp-cmds.c | 2 +- replay/replay-char.c | 4 +-- replay/replay-events.c | 10 +++--- softmmu/bootdevice.c | 4 +-- softmmu/dma-helpers.c| 4 +-- softmmu/memory_mapping.c
[PATCH v2 1/3] scripts/coccinelle: New use-g_new-etc.cocci
This is the semantic patch from commit b45c03f585 "arm: Use g_new() & friends where that makes obvious sense". Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Reviewed-by: Alex Bennée --- scripts/coccinelle/use-g_new-etc.cocci | 75 ++ 1 file changed, 75 insertions(+) create mode 100644 scripts/coccinelle/use-g_new-etc.cocci diff --git a/scripts/coccinelle/use-g_new-etc.cocci b/scripts/coccinelle/use-g_new-etc.cocci new file mode 100644 index 00..e2280e93b3 --- /dev/null +++ b/scripts/coccinelle/use-g_new-etc.cocci @@ -0,0 +1,75 @@ +// Use g_new() & friends where that makes obvious sense +@@ +type T; +@@ +-g_malloc(sizeof(T)) ++g_new(T, 1) +@@ +type T; +@@ +-g_try_malloc(sizeof(T)) ++g_try_new(T, 1) +@@ +type T; +@@ +-g_malloc0(sizeof(T)) ++g_new0(T, 1) +@@ +type T; +@@ +-g_try_malloc0(sizeof(T)) ++g_try_new0(T, 1) +@@ +type T; +expression n; +@@ +-g_malloc(sizeof(T) * (n)) ++g_new(T, n) +@@ +type T; +expression n; +@@ +-g_try_malloc(sizeof(T) * (n)) ++g_try_new(T, n) +@@ +type T; +expression n; +@@ +-g_malloc0(sizeof(T) * (n)) ++g_new0(T, n) +@@ +type T; +expression n; +@@ +-g_try_malloc0(sizeof(T) * (n)) ++g_try_new0(T, n) +@@ +type T; +expression p, n; +@@ +-g_realloc(p, sizeof(T) * (n)) ++g_renew(T, p, n) +@@ +type T; +expression p, n; +@@ +-g_try_realloc(p, sizeof(T) * (n)) ++g_try_renew(T, p, n) +@@ +type T; +expression n; +@@ +-(T *)g_new(T, n) ++g_new(T, n) +@@ +type T; +expression n; +@@ +-(T *)g_new0(T, n) ++g_new0(T, n) +@@ +type T; +expression p, n; +@@ +-(T *)g_renew(T, p, n) ++g_renew(T, p, n) -- 2.35.1
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Tue, 15 Mar 2022 at 14:09, Stefan Hajnoczi wrote: > Also, once C++ is available people will > start submitting C++ patches simply because they are more comfortable > with C++ (especially one-time/infrequent contributors). This to my mind is the major argument against using C++ for coroutines... -- PMM
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
On 15/3/22 14:59, Markus Armbruster wrote:
Alex Bennée writes:
Markus Armbruster writes:
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
for two reasons. One, it catches multiplication overflowing size_t.
Two, it returns T * rather than void *, which lets the compiler catch
more type errors.
diff --git a/semihosting/config.c b/semihosting/config.c
index 137171b717..6d48ec9566 100644
--- a/semihosting/config.c
+++ b/semihosting/config.c
@@ -98,7 +98,7 @@ static int add_semihosting_arg(void *opaque,
if (strcmp(name, "arg") == 0) {
s->argc++;
/* one extra element as g_strjoinv() expects NULL-terminated array */
-s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
+s->argv = g_renew(void *, s->argv, s->argc + 1);
This did indeed break CI because s->argv is an array of *char:
../semihosting/config.c:101:17: error: assignment to ‘const char **’ from
incompatible pointer type ‘void **’ [-Werror=incompatible-pointer-types]
101 | s->argv = g_renew(void *, s->argv, s->argc + 1);
| ^
cc1: all warnings being treated as errors
So it did the job of type checking but failed to build ;-)
You found a hole in my compile testing, thanks!
I got confused about the configuration of my build trees. Catching such
mistakes is what CI is for :)
FYI Alex fixed this here:
https://lore.kernel.org/qemu-devel/20220315121251.2280317-8-alex.ben...@linaro.org/
So your series could go on top (modulo the Coverity change).
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Mon, Mar 14, 2022 at 05:21:22PM +0100, Paolo Bonzini wrote: > On 3/14/22 15:07, Stefan Hajnoczi wrote: > > If we can reach a consensus about C++ language usage in QEMU then I'm in > > favor of using C++ coroutines. It's probably not realistic to think we > > can limit C++ language usage to just coroutines forever. Someone finds > > another C++ feature they absolutely need and over time the codebase > > becomes C++ - with both its advantages and disadvantages. > > > > [...] although you can write C in C++, it's not idiomatic modern C++. > > The language lends itself to a different style of programming that > > some will embrace while others will not. > > Yes, this is an important aspect to discuss. I think coroutines provide a > good blueprint for how QEMU might use C++. > > I totally agree that, if we go this way, the genie is out of the bottle and > other uses of C++ will pop up with 100% probability. But the important > thing to note is that our dialect of C is already not standard C, and that > some of our or GLib's "innovations" are actually based on experience with > C++. We can keep on writing "QEMU's C" if we think of C++ as a supercharged > way of writing these quality-of-life improvements that we already write. In > some sense coroutines are an extreme example of this idea. > > In fact, a C API would have to remain unless all source files are changed to > C++, so QEMU would remain mostly a C project with C idioms, but that doesn't > prevent _abstracting_ the use of C++ features (written in modern, idiomatic > C++) behind an API that C programmers have no problems learning. Again, > coroutines are an example of this of keeping the familiar create/enter/yield > API and hiding the "magic" of C++ coroutines (and when they don't, that had > better be an improvement). > > In the end, C++ is a tool that you can use if it leads to better code. For > example, I don't see much use of C++ for devices for example, and the > storage devices in particular do not even need coroutines because they use > the callback-based interface. But perhaps someone will try to use templates > to replace repeated inclusion (which is common in hw/display) and others > will follow suit. Or perhaps not. > > One example that was brought up on IRC is type-safe operations on things > such as hwaddr (i.e. hwaddr+int is allowed but hwaddr-hwaddr gives back an > int64_t and might even check for overflow). These would be opt in (you get > them just by changing a file from .c to .cc), but the actual C++ code would > still look very much like C code that uses hwaddr with no type checking. > All the operator overloading gunk would be in include/. > > A different topic is what would happen if all of QEMU could be compiled as > C++, and could inform our selection of C++ idioms even long before we get > there. For example, I'm fine with GLib and our type-safe intrusive lists, > so I don't have much interest in STL containers and I would prefer _not_ to > use STL containers even in .cc files[1]. However, perhaps QEMU's home-grown > lock guard might be replaced by something that uses C++ destructors instead > of g_autoptr, so perhaps we should consider using std::lock_guard<>, or > something like that, instead of QEMU_LOCK_GUARD. It may be interesting to > pass down lock_guards as arguments to enforce "this lock is taken" > invariants. > > But really, coroutines would be enough work so my dish would be full for > some time and I wouldn't really have time to look at any of this. :) I think it will be necessary to compile QEMU with a C++ compiler quite soon. It is possible to provide C APIs like in the case of coroutines, but sometimes C++ features need to be exposed to the caller (like the lock guards you mentioned). Also, once C++ is available people will start submitting C++ patches simply because they are more comfortable with C++ (especially one-time/infrequent contributors). We need to agree on a policy so that people know how and when they can use C++. The policy needs to be simple so it doesn't create hurdles for new contributors. Does anyone have experience with C projects that introduced C++ and what worked/didn't work? Stefan signature.asc Description: PGP signature
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Eric Blake writes:
> On Mon, Mar 14, 2022 at 05:01:08PM +0100, Markus Armbruster wrote:
>> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
>> for two reasons. One, it catches multiplication overflowing size_t.
>> Two, it returns T * rather than void *, which lets the compiler catch
>> more type errors.
>>
>> This commit only touches allocations with size arguments of the form
>> sizeof(T).
>>
>> Patch created mechanically with:
>>
>> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>> --macro-file scripts/cocci-macro-file.h FILES...
>>
>> Signed-off-by: Markus Armbruster
>> ---
>
> I agree that this is mechanical, but...
>
>
>> qga/commands-win32.c | 8 ++---
>> qga/commands.c | 2 +-
>> qom/qom-qmp-cmds.c | 2 +-
>> replay/replay-char.c | 4 +--
>> replay/replay-events.c | 10 +++---
>> scripts/coverity-scan/model.c| 2 +-
>
> ...are we sure we want to touch this particular file?
Good catch!
>> diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
>> index 9d4fba53d9..30bea672e1 100644
>> --- a/scripts/coverity-scan/model.c
>> +++ b/scripts/coverity-scan/model.c
>> @@ -356,7 +356,7 @@ int g_poll (GPollFD *fds, unsigned nfds, int timeout)
>> typedef struct _GIOChannel GIOChannel;
>> GIOChannel *g_io_channel_unix_new(int fd)
>> {
>> -GIOChannel *c = g_malloc0(sizeof(GIOChannel));
>> +GIOChannel *c = g_new0(GIOChannel, 1);
>> __coverity_escape__(fd);
>> return c;
>> }
>
> Our model has a definition of g_malloc0(), but I'm not sure whether
> Coverity picks up the macro g_new0() in the same manner.
I believe it does, by parsing the macro definition from the header.
Regardless, I'd prefer to keep model.c self-contained. I'll drop this
hunk.
Thanks!
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Alex Bennée writes:
> Markus Armbruster writes:
>
>> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
>> for two reasons. One, it catches multiplication overflowing size_t.
>> Two, it returns T * rather than void *, which lets the compiler catch
>> more type errors.
>>
>
>> diff --git a/semihosting/config.c b/semihosting/config.c
>> index 137171b717..6d48ec9566 100644
>> --- a/semihosting/config.c
>> +++ b/semihosting/config.c
>> @@ -98,7 +98,7 @@ static int add_semihosting_arg(void *opaque,
>> if (strcmp(name, "arg") == 0) {
>> s->argc++;
>> /* one extra element as g_strjoinv() expects NULL-terminated array
>> */
>> -s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
>> +s->argv = g_renew(void *, s->argv, s->argc + 1);
>
> This did indeed break CI because s->argv is an array of *char:
>
> ../semihosting/config.c:101:17: error: assignment to ‘const char **’ from
> incompatible pointer type ‘void **’ [-Werror=incompatible-pointer-types]
> 101 | s->argv = g_renew(void *, s->argv, s->argc + 1);
> | ^
> cc1: all warnings being treated as errors
>
> So it did the job of type checking but failed to build ;-)
You found a hole in my compile testing, thanks!
I got confused about the configuration of my build trees. Catching such
mistakes is what CI is for :)
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Alex Bennée writes:
> Markus Armbruster writes:
>
>> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
>> for two reasons. One, it catches multiplication overflowing size_t.
>> Two, it returns T * rather than void *, which lets the compiler catch
>> more type errors.
>>
>> This commit only touches allocations with size arguments of the form
>> sizeof(T).
>>
>> Patch created mechanically with:
>>
>> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>> --macro-file scripts/cocci-macro-file.h FILES...
>>
>> Signed-off-by: Markus Armbruster
[...]
>> --- a/hw/pci/pcie_sriov.c
>> +++ b/hw/pci/pcie_sriov.c
>> @@ -177,7 +177,7 @@ static void register_vfs(PCIDevice *dev)
>> assert(sriov_cap > 0);
>> num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
>>
>> -dev->exp.sriov_pf.vf = g_malloc(sizeof(PCIDevice *) * num_vfs);
>> +dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
>> assert(dev->exp.sriov_pf.vf);
>
> So what I find confusing about the conversion of sizeof(foo *) is that
> while the internal sizeof in g_new is unaffected I think the casting
> ends up as
>
> foo **
Yes. g_malloc(...) returns void *. g_new(T, ...) returns T *.
> but I guess the compiler would have complained so maybe I don't
> understand the magic enough.
It doesn't complain here because dev->exp.sriov_pf.vf is of type
PCIDevice **:
struct PCIESriovPF {
uint16_t num_vfs; /* Number of virtual functions created */
uint8_t vf_bar_type[PCI_NUM_REGIONS]; /* Store type for each VF bar */
const char *vfname; /* Reference to the device type used for the VFs */
PCIDevice **vf; /* Pointer to an array of num_vfs VF devices */
};
It does complain when the types don't match, as shown in PATCH 2.
>
>> index 42130667a7..598e6adc5e 100644
>> --- a/hw/rdma/vmw/pvrdma_dev_ring.c
>> +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
>> @@ -41,7 +41,7 @@ int pvrdma_ring_init(PvrdmaRing *ring, const char *name,
>> PCIDevice *dev,
>> qatomic_set(&ring->ring_state->cons_head, 0);
>> */
>> ring->npages = npages;
>> -ring->pages = g_malloc0(npages * sizeof(void *));
>> +ring->pages = g_new0(void *, npages);
>
> At least here ring->pages agrees about void ** being the result.
>
>
>
> So other than my queries about the sizeof(foo *) which I'd like someone
> to assuage me of my concerns it looks like the script has done a
> thorough mechanical job as all good scripts should ;-)
>
> Reviewed-by: Alex Bennée
Thanks!
Re: [PATCH v2 6/6] libvduse: Add support for reconnecting
On Tue, Feb 15, 2022 at 06:59:43PM +0800, Xie Yongji wrote:
> +static int vduse_queue_inflight_get(VduseVirtq *vq, int desc_idx)
> +{
> +vq->log->inflight.desc[desc_idx].counter = vq->counter++;
> +vq->log->inflight.desc[desc_idx].inflight = 1;
Is a barrier needed between these two lines to prevent inflight = 1 with
an undefined counter value?
signature.asc
Description: PGP signature
Re: [PATCH v2 5/6] vduse-blk: Add vduse-blk resize support
On Tue, Feb 15, 2022 at 06:59:42PM +0800, Xie Yongji wrote: > To support block resize, this uses vduse_dev_update_config() > to update the capacity field in configuration space and inject > config interrupt on the block resize callback. > > Signed-off-by: Xie Yongji > --- > block/export/vduse-blk.c | 20 > 1 file changed, 20 insertions(+) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH v3 3/3] nbd/server: Allow MULTI_CONN for shared writable exports
The patches seem OK to me, but I don't really know enough about the internals of qemu-nbd to give a line-by-line review. I did however build and test qemu-nbd with the patches: $ ./build/qemu-nbd /var/tmp/test.qcow2 $ nbdinfo nbd://localhost ... can_multi_conn: false $ ./build/qemu-nbd -e 2 /var/tmp/test.qcow2 $ nbdinfo nbd://localhost ... can_multi_conn: false ^^^ Is this expected? It also happens with -e 0. $ ./build/qemu-nbd -e 2 -m on /var/tmp/test.qcow2 $ nbdinfo nbd://localhost ... can_multi_conn: true $ ./build/qemu-nbd -e 2 -m off /var/tmp/test.qcow2 $ nbdinfo nbd://localhost ... can_multi_conn: false Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Re: [PATCH v7 06/22] hvf: Fix OOB write in RDTSCP instruction decode
On 15/3/22 13:58, Peter Maydell wrote: On Wed, 9 Mar 2022 at 10:20, Philippe Mathieu-Daudé wrote: Hi Paolo, I forgot to Cc you. Could you Ack this patch? I had review comments on the version of this patch in v5 which still need to be addressed: https://lore.kernel.org/qemu-devel/CAFEAcA8yaBOD3KXc-DY94oqzC5wkCENPkePgVCybqR=9nmd...@mail.gmail.com/ Thanks for pointing at your comments, I totally missed them. I'll let Cameron address them. Regards, Phil.
Re: [PATCH v7 06/22] hvf: Fix OOB write in RDTSCP instruction decode
On Wed, 9 Mar 2022 at 10:20, Philippe Mathieu-Daudé wrote: > > Hi Paolo, > > I forgot to Cc you. Could you Ack this patch? I had review comments on the version of this patch in v5 which still need to be addressed: https://lore.kernel.org/qemu-devel/CAFEAcA8yaBOD3KXc-DY94oqzC5wkCENPkePgVCybqR=9nmd...@mail.gmail.com/ thanks -- PMM
Re: [PATCH v7 00/22] host: Support macOS 12
On 7/3/22 00:17, Philippe Mathieu-Daudé wrote: From: Philippe Mathieu-Daudé Few patches to be able to build QEMU on macOS 12 (Monterey). Missing review: 0006-hvf-Fix-OOB-write-in-RDTSCP-instruction-decode.patch 0013-osdep-Avoid-using-Clang-specific-__builtin_available.patch 0014-meson-Resolve-the-entitlement.sh-script-once-for-goo.patch 0015-meson-Log-QEMU_CXXFLAGS-content-in-summary.patch 0016-configure-Pass-filtered-QEMU_OBJCFLAGS-to-meson.patch 0017-ui-cocoa-Constify-qkeycode-translation-arrays.patch 0020-ui-cocoa-capture-all-keys-and-combos-when-mouse-is-g.patch 0021-ui-cocoa-add-option-to-swap-Option-and-Command.patch 0022-gitlab-ci-Support-macOS-12-via-cirrus-run.patch Queuing reviewed patches; IOW all except: 0006-hvf-Fix-OOB-write-in-RDTSCP-instruction-decode.patch 0022-gitlab-ci-Support-macOS-12-via-cirrus-run.patch Thanks, Phil.
Re: [PATCH v2 4/6] vduse-blk: implements vduse-blk export
On Tue, Mar 15, 2022 at 7:08 PM Stefan Hajnoczi wrote:
>
> On Tue, Feb 15, 2022 at 06:59:41PM +0800, Xie Yongji wrote:
> > This implements a VDUSE block backends based on
> > the libvduse library. We can use it to export the BDSs
> > for both VM and container (host) usage.
> >
> > The new command-line syntax is:
> >
> > $ qemu-storage-daemon \
> > --blockdev file,node-name=drive0,filename=test.img \
> > --export vduse-blk,node-name=drive0,id=vduse-export0,writable=on
> >
> > After the qemu-storage-daemon started, we need to use
> > the "vdpa" command to attach the device to vDPA bus:
> >
> > $ vdpa dev add name vduse-export0 mgmtdev vduse
>
> The per-QEMU export id is used as the global vdpa device name. If this
> becomes a problem in the future then a new --export
> vduse-blk,vdpa-dev-name= option can be added.
>
Yes.
> > +case VIRTIO_BLK_T_GET_ID: {
> > +size_t size = MIN(iov_size(&elem->in_sg[0], in_num),
> > + VIRTIO_BLK_ID_BYTES);
> > +snprintf(elem->in_sg[0].iov_base, size, "%s", vblk_exp->export.id);
>
> Please use iov_from_buf(). The driver is allowed to submit as many
> in_sg[] elements as it wants and a compliant virtio-blk device
> implementation must support that.
>
Got it.
> Here is the VIRTIO specification section that covers message framing:
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-280004
>
> > +features = (1ULL << VIRTIO_F_IOMMU_PLATFORM) |
> > + (1ULL << VIRTIO_F_VERSION_1) |
> > + (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> > + (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
> > + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> > + (1ULL << VIRTIO_BLK_F_SIZE_MAX) |
> > + (1ULL << VIRTIO_BLK_F_SEG_MAX) |
> > + (1ULL << VIRTIO_BLK_F_TOPOLOGY) |
> > + (1ULL << VIRTIO_BLK_F_BLK_SIZE);
>
> The VIRTIO_F_ and VIRTIO_RING_F_ feature bits report the capabilities of
> libvduse. They should probably be defined in libvduse. That way no
> changes to vduse-blk.c are required when libvduse changes:
>
> features = LIBVDUSE_VIRTIO_FEATURES |
> (1ULL << VIRTIO_BLK_F_SIZE_MAX) |
> ...;
It's OK to define the VIRTIO_F_* feature bits in libvduse since daemon
might not want to disable it. But maybe not including VIRTIO_RING_F_*
feature bits since daemon might want to disable them in some cases.
Thanks,
Yongji
Re: [PATCH v2 3/6] libvduse: Add VDUSE (vDPA Device in Userspace) library
On Tue, Mar 15, 2022 at 5:48 PM Stefan Hajnoczi wrote: > > On Tue, Feb 15, 2022 at 06:59:40PM +0800, Xie Yongji wrote: > > VDUSE [1] is a linux framework that makes it possible to implement > > software-emulated vDPA devices in userspace. This adds a library > > as a subproject to help implementing VDUSE backends in QEMU. > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/vduse.html > > > > Signed-off-by: Xie Yongji > > --- > > meson.build | 15 + > > meson_options.txt |2 + > > scripts/meson-buildoptions.sh |3 + > > subprojects/libvduse/include/atomic.h |1 + > > subprojects/libvduse/libvduse.c | 1152 +++ > > subprojects/libvduse/libvduse.h | 225 > > subprojects/libvduse/linux-headers/linux|1 + > > subprojects/libvduse/meson.build| 10 + > > subprojects/libvduse/standard-headers/linux |1 + > > 9 files changed, 1410 insertions(+) > > create mode 12 subprojects/libvduse/include/atomic.h > > create mode 100644 subprojects/libvduse/libvduse.c > > create mode 100644 subprojects/libvduse/libvduse.h > > create mode 12 subprojects/libvduse/linux-headers/linux > > create mode 100644 subprojects/libvduse/meson.build > > create mode 12 subprojects/libvduse/standard-headers/linux > > Please update the ./MAINTAINERS file when adding new source files. OK, sure. And would you mind being one of the maintainers since I'm not sure if I can do this job well. Thanks, Yongji
Re: [PATCH v2 4/6] vduse-blk: implements vduse-blk export
On Tue, Feb 15, 2022 at 06:59:41PM +0800, Xie Yongji wrote:
> This implements a VDUSE block backends based on
> the libvduse library. We can use it to export the BDSs
> for both VM and container (host) usage.
>
> The new command-line syntax is:
>
> $ qemu-storage-daemon \
> --blockdev file,node-name=drive0,filename=test.img \
> --export vduse-blk,node-name=drive0,id=vduse-export0,writable=on
>
> After the qemu-storage-daemon started, we need to use
> the "vdpa" command to attach the device to vDPA bus:
>
> $ vdpa dev add name vduse-export0 mgmtdev vduse
The per-QEMU export id is used as the global vdpa device name. If this
becomes a problem in the future then a new --export
vduse-blk,vdpa-dev-name= option can be added.
> +case VIRTIO_BLK_T_GET_ID: {
> +size_t size = MIN(iov_size(&elem->in_sg[0], in_num),
> + VIRTIO_BLK_ID_BYTES);
> +snprintf(elem->in_sg[0].iov_base, size, "%s", vblk_exp->export.id);
Please use iov_from_buf(). The driver is allowed to submit as many
in_sg[] elements as it wants and a compliant virtio-blk device
implementation must support that.
Here is the VIRTIO specification section that covers message framing:
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-280004
> +features = (1ULL << VIRTIO_F_IOMMU_PLATFORM) |
> + (1ULL << VIRTIO_F_VERSION_1) |
> + (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> + (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
> + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> + (1ULL << VIRTIO_BLK_F_SIZE_MAX) |
> + (1ULL << VIRTIO_BLK_F_SEG_MAX) |
> + (1ULL << VIRTIO_BLK_F_TOPOLOGY) |
> + (1ULL << VIRTIO_BLK_F_BLK_SIZE);
The VIRTIO_F_ and VIRTIO_RING_F_ feature bits report the capabilities of
libvduse. They should probably be defined in libvduse. That way no
changes to vduse-blk.c are required when libvduse changes:
features = LIBVDUSE_VIRTIO_FEATURES |
(1ULL << VIRTIO_BLK_F_SIZE_MAX) |
...;
signature.asc
Description: PGP signature
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
* Markus Armbruster (arm...@redhat.com) wrote:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> This commit only touches allocations with size arguments of the form
> sizeof(T).
>
> Patch created mechanically with:
>
> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>--macro-file scripts/cocci-macro-file.h FILES...
>
> Signed-off-by: Markus Armbruster
Just a small patch then...
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index d65e744af9..aace12a787 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -91,7 +91,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
> {
> int i;
> int64_t dirty_rate = DirtyStat.dirty_rate;
> -struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
> +struct DirtyRateInfo *info = g_new0(DirtyRateInfo, 1);
> DirtyRateVcpuList *head = NULL, **tail = &head;
>
> info->status = CalculatingState;
> @@ -112,7 +112,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
> info->sample_pages = 0;
> info->has_vcpu_dirty_rate = true;
> for (i = 0; i < DirtyStat.dirty_ring.nvcpu; i++) {
> -DirtyRateVcpu *rate = g_malloc0(sizeof(DirtyRateVcpu));
> +DirtyRateVcpu *rate = g_new0(DirtyRateVcpu, 1);
> rate->id = DirtyStat.dirty_ring.rates[i].id;
> rate->dirty_rate = DirtyStat.dirty_ring.rates[i].dirty_rate;
> QAPI_LIST_APPEND(tail, rate);
> diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
> index aba1c88a0c..3a7ae44485 100644
> --- a/migration/multifd-zlib.c
> +++ b/migration/multifd-zlib.c
> @@ -43,7 +43,7 @@ struct zlib_data {
> */
> static int zlib_send_setup(MultiFDSendParams *p, Error **errp)
> {
> -struct zlib_data *z = g_malloc0(sizeof(struct zlib_data));
> +struct zlib_data *z = g_new0(struct zlib_data, 1);
> z_stream *zs = &z->zs;
>
> zs->zalloc = Z_NULL;
> @@ -164,7 +164,7 @@ static int zlib_send_prepare(MultiFDSendParams *p, Error
> **errp)
> */
> static int zlib_recv_setup(MultiFDRecvParams *p, Error **errp)
> {
> -struct zlib_data *z = g_malloc0(sizeof(struct zlib_data));
> +struct zlib_data *z = g_new0(struct zlib_data, 1);
> z_stream *zs = &z->zs;
>
> p->data = z;
> diff --git a/migration/ram.c b/migration/ram.c
> index 170e522a1f..3532f64ecb 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2059,7 +2059,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t
> start, ram_addr_t len)
> }
>
> struct RAMSrcPageRequest *new_entry =
> -g_malloc0(sizeof(struct RAMSrcPageRequest));
> +g_new0(struct RAMSrcPageRequest, 1);
> new_entry->rb = ramblock;
> new_entry->offset = start;
> new_entry->len = len;
For migration:
Acked-by: Dr. David Alan Gilbert
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH v2] meson: Update to version 0.61.3
Meson 0.61.3 contains an important fix which helps to see the
output of failed qemu-iotests on the console again:
https://gitlab.com/qemu-project/meson/-/commit/7534cf34f83b9c43
Acked-by: Paolo Bonzini
Signed-off-by: Thomas Huth
---
v2: Update the version in the configure script, too (as suggested by Paolo)
configure | 2 +-
meson | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/configure b/configure
index 886000346a..8e3c41ff92 100755
--- a/configure
+++ b/configure
@@ -1286,7 +1286,7 @@ python_version=$($python -c 'import sys; print("%d.%d.%d"
% (sys.version_info[0]
python="$python -B"
if test -z "$meson"; then
-if test "$explicit_python" = no && has meson && version_ge "$(meson
--version)" 0.59.3; then
+if test "$explicit_python" = no && has meson && version_ge "$(meson
--version)" 0.61.3; then
meson=meson
elif test $git_submodules_action != 'ignore' ; then
meson=git
diff --git a/meson b/meson
index 12f9f04ba0..5cf5575a7c 16
--- a/meson
+++ b/meson
@@ -1 +1 @@
-Subproject commit 12f9f04ba0decfda425dbbf9a501084c153a2d18
+Subproject commit 5cf5575a7c76746935dcd9a9e380803c85023c04
--
2.27.0
Re: [PATCH v2 3/6] libvduse: Add VDUSE (vDPA Device in Userspace) library
On Tue, Feb 15, 2022 at 06:59:40PM +0800, Xie Yongji wrote: > VDUSE [1] is a linux framework that makes it possible to implement > software-emulated vDPA devices in userspace. This adds a library > as a subproject to help implementing VDUSE backends in QEMU. > > [1] https://www.kernel.org/doc/html/latest/userspace-api/vduse.html > > Signed-off-by: Xie Yongji > --- > meson.build | 15 + > meson_options.txt |2 + > scripts/meson-buildoptions.sh |3 + > subprojects/libvduse/include/atomic.h |1 + > subprojects/libvduse/libvduse.c | 1152 +++ > subprojects/libvduse/libvduse.h | 225 > subprojects/libvduse/linux-headers/linux|1 + > subprojects/libvduse/meson.build| 10 + > subprojects/libvduse/standard-headers/linux |1 + > 9 files changed, 1410 insertions(+) > create mode 12 subprojects/libvduse/include/atomic.h > create mode 100644 subprojects/libvduse/libvduse.c > create mode 100644 subprojects/libvduse/libvduse.h > create mode 12 subprojects/libvduse/linux-headers/linux > create mode 100644 subprojects/libvduse/meson.build > create mode 12 subprojects/libvduse/standard-headers/linux Please update the ./MAINTAINERS file when adding new source files. signature.asc Description: PGP signature
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Tue, Mar 15, 2022 at 10:05:32AM +0100, Paolo Bonzini wrote: > On 3/14/22 17:52, Daniel P. Berrangé wrote: > > RHEL-8: 10.0.1 > > openSUSE Leap 15.3: 9.0.1 > >Ubuntu LTS 18.04: 6.0.0 > > FreeBSD 12: 8.0.1 > > > > Ubuntu 18.04 drops off our list after 7.0 comes out > > > > OpenSUSE Leap 15.2 was EOL'd by SUSE themselves in Jan 2022, > > We use it as a proxy for SLES, but I think we can required > > SLES 15 sp3. > > (FTR, OpenSUSE Leap 15.3 has GCC 10.3.0). > > > FreeBSD 12 is something we still support until April 2023, > > but arguably we only care about CLang there. > > > > NetBSD 9 wasn't listed, but it was reported to required > > GCC 7.4 (commit 3830df5f83b9b52d9496763ce1a50afb9231c998) > > and that is still the latest release of NetBSD. > > > > So NetBSD is our biggest constraint on requiring GCC 10 > > Do we care about the BSDs since they have newer compilers (including gcc10) > available in pkgsrc? If you go by the base system, then RHEL8 has 8.5.0 and > newer version are only available with packages such as gcc-toolset-10 and > gcc-toolset-11. I mention NetBSD because we had an explicit request to support 7.4 GCC from there, as it was the current system compiler. That's a mistake in my original commit logs then wrt RHEL8, as I only ever intended to consider the standard base repos, not random addon repos. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
On Mon, Mar 14, 2022 at 05:01:08PM +0100, Markus Armbruster wrote:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> This commit only touches allocations with size arguments of the form
> sizeof(T).
>
> Patch created mechanically with:
>
> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>--macro-file scripts/cocci-macro-file.h FILES...
>
> Signed-off-by: Markus Armbruster
> ---
I agree that this is mechanical, but...
> qga/commands-win32.c | 8 ++---
> qga/commands.c | 2 +-
> qom/qom-qmp-cmds.c | 2 +-
> replay/replay-char.c | 4 +--
> replay/replay-events.c | 10 +++---
> scripts/coverity-scan/model.c| 2 +-
...are we sure we want to touch this particular file?
> diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
> index 9d4fba53d9..30bea672e1 100644
> --- a/scripts/coverity-scan/model.c
> +++ b/scripts/coverity-scan/model.c
> @@ -356,7 +356,7 @@ int g_poll (GPollFD *fds, unsigned nfds, int timeout)
> typedef struct _GIOChannel GIOChannel;
> GIOChannel *g_io_channel_unix_new(int fd)
> {
> -GIOChannel *c = g_malloc0(sizeof(GIOChannel));
> +GIOChannel *c = g_new0(GIOChannel, 1);
> __coverity_escape__(fd);
> return c;
> }
Our model has a definition of g_malloc0(), but I'm not sure whether
Coverity picks up the macro g_new0() in the same manner.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH for-7.0?] meson: Update to version 0.61.3
On 3/15/22 09:35, Thomas Huth wrote: Meson 0.61.3 contains an important fix which helps to see the output of failed qemu-iotests on the console again: https://gitlab.com/qemu-project/meson/-/commit/7534cf34f83b9c43 Signed-off-by: Thomas Huth --- meson | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson b/meson index 12f9f04ba0..5cf5575a7c 16 --- a/meson +++ b/meson @@ -1 +1 @@ -Subproject commit 12f9f04ba0decfda425dbbf9a501084c153a2d18 +Subproject commit 5cf5575a7c76746935dcd9a9e380803c85023c04 Please update configure as well: -if test "$explicit_python" = no && has meson && version_ge "$(meson --version)" 0.59.3; then +if test "$explicit_python" = no && has meson && version_ge "$(meson --version)" 0.61.3; then The bump would only apply if --python is specified or if --meson is not specified; therefore, it wouldn't change the minimum supported version. Acked-by: Paolo Bonzini Paolo
Re: [PATCH experiment 00/16] C++20 coroutine backend
On 3/14/22 17:52, Daniel P. Berrangé wrote: RHEL-8: 10.0.1 openSUSE Leap 15.3: 9.0.1 Ubuntu LTS 18.04: 6.0.0 FreeBSD 12: 8.0.1 Ubuntu 18.04 drops off our list after 7.0 comes out OpenSUSE Leap 15.2 was EOL'd by SUSE themselves in Jan 2022, We use it as a proxy for SLES, but I think we can required SLES 15 sp3. (FTR, OpenSUSE Leap 15.3 has GCC 10.3.0). FreeBSD 12 is something we still support until April 2023, but arguably we only care about CLang there. NetBSD 9 wasn't listed, but it was reported to required GCC 7.4 (commit 3830df5f83b9b52d9496763ce1a50afb9231c998) and that is still the latest release of NetBSD. So NetBSD is our biggest constraint on requiring GCC 10 Do we care about the BSDs since they have newer compilers (including gcc10) available in pkgsrc? If you go by the base system, then RHEL8 has 8.5.0 and newer version are only available with packages such as gcc-toolset-10 and gcc-toolset-11. Paolo
Re: [PATCH v2 1/6] block: Support passing NULL ops to blk_set_dev_ops()
On Mon, Mar 14, 2022 at 03:09:35PM -0400, John Snow wrote:
> On Mon, Mar 14, 2022 at 1:23 PM Stefan Hajnoczi wrote:
> >
> > On Tue, Feb 15, 2022 at 06:59:38PM +0800, Xie Yongji wrote:
> > > This supports passing NULL ops to blk_set_dev_ops()
> > > so that we can remove stale ops in some cases.
> > >
> > > Signed-off-by: Xie Yongji
> > > ---
> > > block/block-backend.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > index 4ff6b4d785..08dd0a3093 100644
> > > --- a/block/block-backend.c
> > > +++ b/block/block-backend.c
> > > @@ -1015,7 +1015,7 @@ void blk_set_dev_ops(BlockBackend *blk, const
> > > BlockDevOps *ops,
> > > blk->dev_opaque = opaque;
> > >
> > > /* Are we currently quiesced? Should we enforce this right now? */
> > > -if (blk->quiesce_counter && ops->drained_begin) {
> > > +if (blk->quiesce_counter && ops && ops->drained_begin) {
> > > ops->drained_begin(opaque);
> > > }
> > > }
> >
> > John: You added this code in f4d9cc88ee6. Does blk_set_dev_ops() need to
> > call ->drained_end() when ops is set to NULL?
> >
> > Stefan
>
> I'm not sure I trust my memory from five years ago.
>
> From what I recall, the problem was that block jobs weren't getting
> drained/paused when the backend was getting quiesced -- we wanted to
> be sure that a blockjob wasn't continuing to run and submit requests
> against a backend we wanted to have on ice during a sensitive
> operation. This conditional stanza here is meant to check if the node
> we're already attached to is *already quiesced* and we missed the
> signal (so-to-speak), so we replay the drained_begin() request right
> there.
>
> i.e. there was some case where blockjobs were getting added to an
> already quiesced node, and this code here post-hoc relays that drain
> request to the blockjob. This gets used in
> 600ac6a0ef5c06418446ef2f37407bddcc51b21c to pause/unpause jobs.
> Original thread is here:
> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg03416.html
>
> Now, I'm not sure why you want to set ops to NULL here. If we're in a
> drained section, that sounds like it might be potentially bad because
> we lose track of the operation to end the drained section. If your
> intent is to destroy the thing that we'd need to call drained_end on,
> I guess it doesn't matter -- provided you've cleaned up the target
> object correctly. Just calling drained_end() pre-emptively seems like
> it might be bad, what if it unpauses something you're in the middle of
> trying to delete?
>
> I might need slightly more context to know what you're hoping to
> accomplish, but I hope this info helps contextualize this code
> somewhat.
Setting to NULL in this patch is a subset of blk_detach_dev(), which
gets called when a storage controller is hot unplugged.
In this patch series there is no DeviceState because a VDUSE export is
not a device. The VDUSE code calls blk_set_dev_ops() to
register/unregister callbacks (e.g. ->resize_cb()).
The reason I asked about ->drained_end() is for symmetry. If the
device's ->drained_begin() callback changed state or allocated resources
then they may need to be freed/reset. On the other hand, the
blk_set_dev_ops(blk, NULL, NULL) call should be made by the dev_ops
owner so they can clean up without a ->drained_end() call.
Stefan
signature.asc
Description: PGP signature
[PATCH for-7.0?] meson: Update to version 0.61.3
Meson 0.61.3 contains an important fix which helps to see the output of failed qemu-iotests on the console again: https://gitlab.com/qemu-project/meson/-/commit/7534cf34f83b9c43 Signed-off-by: Thomas Huth --- meson | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson b/meson index 12f9f04ba0..5cf5575a7c 16 --- a/meson +++ b/meson @@ -1 +1 @@ -Subproject commit 12f9f04ba0decfda425dbbf9a501084c153a2d18 +Subproject commit 5cf5575a7c76746935dcd9a9e380803c85023c04 -- 2.27.0
Re: [PATCH 2/3] 9pfs: Use g_new() & friends where that makes obvious sense
On Mon, 14 Mar 2022 17:01:07 +0100
Markus Armbruster wrote:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> This commit only touches allocations with size arguments of the form
> sizeof(T).
>
> Patch created mechanically with:
>
> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>--macro-file scripts/cocci-macro-file.h FILES...
>
> Except this uncovers a typing error:
>
> ../hw/9pfs/9p.c:855:13: warning: incompatible pointer types assigning to
> 'QpfEntry *' from 'QppEntry *' [-Wincompatible-pointer-types]
> val = g_new0(QppEntry, 1);
> ^ ~~~
> 1 warning generated.
>
> Harmless, because QppEntry is larger than QpfEntry. Fix to allocate a
> QpfEntry instead.
>
> Cc: Greg Kurz
> Cc: Christian Schoenebeck
> Signed-off-by: Markus Armbruster
> ---
Reviewed-by: Greg Kurz
> hw/9pfs/9p-proxy.c | 2 +-
> hw/9pfs/9p-synth.c | 4 ++--
> hw/9pfs/9p.c | 8
> hw/9pfs/codir.c | 6 +++---
> tests/qtest/virtio-9p-test.c | 4 ++--
> 5 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index 8b4b5cf7dc..4c5e0fc217 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -1187,7 +1187,7 @@ static int proxy_parse_opts(QemuOpts *opts,
> FsDriverEntry *fs, Error **errp)
>
> static int proxy_init(FsContext *ctx, Error **errp)
> {
> -V9fsProxy *proxy = g_malloc(sizeof(V9fsProxy));
> +V9fsProxy *proxy = g_new(V9fsProxy, 1);
> int sock_id;
>
> if (ctx->export_flags & V9FS_PROXY_SOCK_NAME) {
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index b3080e415b..d99d263985 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -49,7 +49,7 @@ static V9fsSynthNode *v9fs_add_dir_node(V9fsSynthNode
> *parent, int mode,
>
> /* Add directory type and remove write bits */
> mode = ((mode & 0777) | S_IFDIR) & ~(S_IWUSR | S_IWGRP | S_IWOTH);
> -node = g_malloc0(sizeof(V9fsSynthNode));
> +node = g_new0(V9fsSynthNode, 1);
> if (attr) {
> /* We are adding .. or . entries */
> node->attr = attr;
> @@ -128,7 +128,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int
> mode,
> }
> /* Add file type and remove write bits */
> mode = ((mode & 0777) | S_IFREG);
> -node = g_malloc0(sizeof(V9fsSynthNode));
> +node = g_new0(V9fsSynthNode, 1);
> node->attr = &node->actual_attr;
> node->attr->inode = synth_node_count++;
> node->attr->nlink = 1;
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index a6d6b3f835..8e9d4aea73 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -324,7 +324,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
> return NULL;
> }
> }
> -f = g_malloc0(sizeof(V9fsFidState));
> +f = g_new0(V9fsFidState, 1);
> f->fid = fid;
> f->fid_type = P9_FID_NONE;
> f->ref = 1;
> @@ -804,7 +804,7 @@ static int qid_inode_prefix_hash_bits(V9fsPDU *pdu, dev_t
> dev)
>
> val = qht_lookup(&pdu->s->qpd_table, &lookup, hash);
> if (!val) {
> -val = g_malloc0(sizeof(QpdEntry));
> +val = g_new0(QpdEntry, 1);
> *val = lookup;
> affix = affixForIndex(pdu->s->qp_affix_next);
> val->prefix_bits = affix.bits;
> @@ -852,7 +852,7 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct
> stat *stbuf,
> return -ENFILE;
> }
>
> -val = g_malloc0(sizeof(QppEntry));
> +val = g_new0(QpfEntry, 1);
> *val = lookup;
>
> /* new unique inode and device combo */
> @@ -928,7 +928,7 @@ static int qid_path_suffixmap(V9fsPDU *pdu, const struct
> stat *stbuf,
> return -ENFILE;
> }
>
> -val = g_malloc0(sizeof(QppEntry));
> +val = g_new0(QppEntry, 1);
> *val = lookup;
>
> /* new unique inode affix and device combo */
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index 75148bc985..93ba44fb75 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -141,9 +141,9 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> *fidp,
>
> /* append next node to result chain */
> if (!e) {
> -*entries = e = g_malloc0(sizeof(V9fsDirEnt));
> +*entries = e = g_new0(V9fsDirEnt, 1);
> } else {
> -e = e->next = g_malloc0(sizeof(V9fsDirEnt));
> +e = e->next = g_new0(V9fsDirEnt, 1);
> }
> e->dent = qemu_dirent_dup(dent);
>
> @@ -163,7 +163,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> *fidp,
> break;
> }
>
> -e->st = g_malloc0(sizeof(struct stat));
> +
