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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Dienstag, 25. Januar 2022 12:08:21 CET Akihiko Odaki wrote:
> 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. 

Yes, AFAICS those NSAutoReleasePools in ui/cocoa.m can safely be removed, as 
they were apparently just fighting the symptoms of having called cocoa APIs 
from non-main thread(s) in the past, which is not allowed.

There is actually a "main thread checker" for that. In Xcode it's just a 
checkbox away, but I don't see a corresponding clang parameter. Maybe it's in 
a separate toolchain by Apple.

However I don't think the NSAutoReleasePools were the cause of the memory leak 
you saw.

> Enabling ARC is a long-term
> solution and would allow clang to analyze object management code and
> answer such a question semi-automatically.

Yeah, that's not trivial and would be a long way. Personally I would say, for 
new targets, sure use ARC. But for already existing, large projects like QEMU, 
I would not switch to ARC. Because it is not just refactoring, you have to 
understand each component and make proper decisions for references (weak, 
strong, copy, bridge transfers, adjust blocks, ... ) and avoid cyclic 
references, otherwise you just replace existing issues with new ones, 
including new leaks.

> 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")

I didn't spot anything negative, but I can't test those patches ATM.

> > 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.

Which reminds me of the automated Xcode project discussion for QEMU. I still 
think if there was a simple way to work on QEMU with Xcode there were plenty 
of macOS reviewers and contributors, and I think it can be done with Meson.

Best regards,
Christian Schoenebeck