Re: [PATCH v13 2/7] net/vmnet: add vmnet backends to qapi/net

From: Akihiko Odaki <akihiko.odaki@gmail.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Vladislav Yaroshchuk" <yaroshchuk2000@gmail.com>,
	phillip.ennen@gmail.com, "Jason Wang" <jasowang@redhat.com>,
	"Christian Schoenebeck" <qemu_oss@crudebyte.com>,
	"qemu Developers" <qemu-devel@nongnu.org>,
	"Cameron Esfahani" <dirty@apple.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"Roman Bolshakov" <r.bolshakov@yadro.com>,
	"Alexander Graf" <agraf@csgraf.de>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Howard Spoelstra" <hsp.cat7@gmail.com>,
	"Roman Bolshakov" <roman@roolebo.dev>,
	"Alessio Dionisi" <hello@adns.io>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Eric Blake" <eblake@redhat.com>,
	"Phillip Tennen" <phillip@axleos.com>
Subject: Re: [PATCH v13 2/7] net/vmnet: add vmnet backends to qapi/net
Date: Tue, 25 Jan 2022 20:08:21 +0900	[thread overview]
Message-ID: <CAMVc7JVShGiPvwc4fWHfG2JjTX0QGOcs3ua3k58WFdo4fOLS6A@mail.gmail.com> (raw)
In-Reply-To: <CAFEAcA_CT6AJx_ns4zjw1_udq-Ab3YdM2mzPcKKZberUPOqhPA@mail.gmail.com>

On Tue, Jan 25, 2022 at 7:32 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 25 Jan 2022 at 04:14, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
> > I'm neutral about the decision. I think QEMU should avoid using
> > Objective-C code except for interactions with Apple's APIs, and .c is
> > superior in terms of that as it would prevent accidental introduction
> > of Objective-C code. On the other hand, naming them .m will allow the
> > introduction of Automatic Reference Counting to manage dispatch queue
> > objects. In fact, I have found a few memory leaks in vmnet in the last
> > review and ui/cocoa.m has a suspicious construction of the object
> > management (Particularly it has asynchronous dispatches wrapped with
> > NSAutoreleasePool, which does not make sense).
>
> I think those are probably my fault -- in commit 6e657e64cd (in 2013)
> we added NSAutoReleasePools to fix leaks that happened because
> we were calling into Cocoa APIs from threads other than the UI
> thread that didn't have their own automatically created autorelease
> pool. Much later in commit 5588840ff778 (in 2019) we put in the
> dispatch_async stuff because newer macOS was stricter about
> requiring Cocoa API calls to be only on the UI thread. So
> I think that means the requirement for the autorelease pools
> has now gone away in those functions and we could simply delete
> them -- does that sound right? (I freely admit that I'm not a macOS
> expert -- I just look stuff up in the documentation; historically
> we haven't really had many expert macOS people around to work on
> cocoa.m...)

Removing them would be an improvement. Enabling ARC is a long-term
solution and would allow clang to analyze object management code and
answer such a question semi-automatically.

Regards,
Akihiko Odaki

>
> On the subject of cocoa.m, while we have various macOS-interested
> people in this thread, can I ask if anybody would like to
> review a couple of patches that came in at the beginning of the
> year?
>
> https://patchew.org/QEMU/20220102174153.70043-1-carwynellis@gmail.com/
> ("ui/cocoa: Add option to disable left command and hide cursor on click")
> and
> https://patchew.org/QEMU/20220103114515.24020-1-carwynellis@gmail.com/
> ("Show/hide the menu bar in fullscreen on mouse")
>
> either from the point of view of "is this a sensible change to
> the macOS UI experience" or for the actual code changes, or both.
>
> We've been very short on upstream macOS code reviewers so if people
> interested in that host platform are able to chip in by
> reviewing each others' code that helps a lot.
>
> thanks
> -- PMM