On 2025/06/06 18:54, Alex Bennée wrote:
Akihiko Odaki <od...@rsg.ci.i.u-tokyo.ac.jp> writes:
On 2025/06/06 1:26, Alex Bennée wrote:
QOM objects can be embedded in other QOM objects and managed as part
of their lifetime but this isn't the case for
virtio_gpu_virgl_hostmem_region. However before we can split it out we
need some other way of associating the wider data structure with the
memory region.
Fortunately MemoryRegion has an opaque pointer. This is passed down
to
MemoryRegionOps for device type regions but is unused in the
memory_region_init_ram_ptr() case. Use the opaque to carry the
reference and allow the final MemoryRegion object to be reaped when
its reference count is cleared.
Signed-off-by: Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Message-Id: <20250410122643.1747913-2-manos.pitsidiana...@linaro.org>
Cc: qemu-sta...@nongnu.org
Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
Message-ID: <20250603110204.838117-10-alex.ben...@linaro.org>
I have told you that you should address all comments before sending a
series again a few times[1][2], but you haven't done that.
I've given reasons. Thanks for your review but you don't get to veto.
I pointed out it has no effect (fixing or improving something) other
than adding a memory allocation, but you didn't make a reply to prove
otherwise.
I explained the commit cover what it is doing.
It still doesn't explain the motivation.
For example, "[PULL 01/17] tests/docker: expose $HOME/.cache/qemu as
docker volume" does explain its motivation. It says:
> If you want to run functional tests we should share .cache/qemu so we
> don't force containers to continually re-download images.
So, with the patch, containers will no longer require continually
re-downloading images, which is nice.
Back to this memory region patch, what does it contribute? What's nice
with this patch?
I also pointed out it leaks memory and you asked for a test case[4],
but you made this pull request without giving me 24 hours to reply to
it.
You keep bringing up theoretical issues. We have passing test cases now
and we have plenty of time to address any bugs we might discover. But
holding onto these patches is slowing down other work getting in and I
don't deem it a risk to merge as is.
Things that involve concurrency and memory safety will often become
theoretical.
For example, I recently changed a concurrency algorithm with the
following patch:
https://lore.kernel.org/qemu-devel/20250526-event-v4-0-5b784cc8e...@daynix.com/
All reviews were theoretical and did not provide any test case. These
reviews still matter and I replied them by providing my theory, which
eventually materialized as a documentation patch:
https://lore.kernel.org/qemu-devel/b41eb6f4-96b8-47bf-90cf-e4918a613...@daynix.com/T/#m2603d613d8d8cbbe87b4dce63fd2663c58d52e55
The situation of "[PULL 03/17] tests/tcg: make aarch64 boot.S handle
different starting modes" is also similar. I added a comment about
symbol naming and you gave a reasoning, but I didn't get time to
review it either[5]. Besides, I also had a suggestion to make the code
shorter for the past version, but it is also dismissed.
I also pointed out "[PULL 11/17] ui/gtk-gl-area: Remove extra draw
call in refresh" has an undressed comment[2][7].
I would like to see improvements in how comments are addressed before
a series is resent.
No - I'm sorry you don't get to veto a pull request because it doesn't
meet your particular standards.
The standards are not mine, but documented in:
docs/devel/submitting-a-patch.rst
I'm happy with the other review and level of testing of the patches to
put it in a pull request. I held off the other well tested patch in the
series out of an abundance of caution but will keep it in the
virtio-gpu/next tree and re-post once I've done my next sweep for my
maintainer trees.
A reviewer don't have a right to veto but the mentioned documentation
says they can still get a submitter's counterargument when the submitter
disagrees with them. Reviews should not be just silently ignored.
Regards,
Akihiko Odaki
