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

[Roman Bolshakov]

On Tue, Jan 25, 2022 at 01:14:27PM +0900, Akihiko Odaki wrote:
> On Tue, Jan 25, 2022 at 8:00 AM Roman Bolshakov <roman@roolebo.dev> wrote:
> >
> > On Mon, Jan 24, 2022 at 08:14:31PM +0000, Peter Maydell wrote:
> > > On Mon, 24 Jan 2022 at 17:49, Roman Bolshakov <roman@roolebo.dev> wrote:
> > > > I'm not sure why blocks are Objective-C specific. All the data I have
> > > > shows the opposite [3][4][5]. They're just extensively used in Apple 
> > > > APIs.
> > >
> > > This is true, but for the purposes of our build machinery it is
> > > simpler to have three types of source files that it deals
> > > with (C, C++, ObjC) rather than four (C, C++, ObjC, C-that-uses-blocks).
> > > So unless there's a clear benefit from adding the extra category
> > > I think we should do the simple thing and keep these files named
> > > with a ".m" extension.
> > >
> >
> > Fine by me as long as majority finds it's simpler :) Perhaps it's just a
> > matter of personal preference.
> >
> > I've used to the fact that platform-specific code uses platform-specific
> > extensions or some sort of weird "GCC attributes". Therefore C with an
> > extension is easier to reason for me than Objective-C with ARC and other
> > kinds of implicit behaviour without an actual Objective-C code.
> >
> 
> Being technically pedantic, actually this vmnet implementation uses
> Objective-C and there is a file with .c which uses blocks.
> If a file is named .m, dispatch_retain(o) will be redefined as [o
> retain], and effectively makes it Objective-C code. Therefore, vmnet
> involves Objective-C as long as its files are named .m. It will be C
> with blocks if they are named .c.
> Speaking of use of blocks, actually audio/coreaudio.c involves blocks
> in header files; Core Audio has functions which accept blocks.
> 

Right, dispatch_retain()/dispatch_release() is just one example of the
implicit behaviour I'm talking about.

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

That was exactly my point :)

> On the other hand, naming them .m will allow the
> introduction of Automatic Reference Counting to manage dispatch queue
> objects.

As of now ARC doesn't work automatically for .m files in QEMU. It
happens because QEMU doesn't enable it via -fobjc-arc.

If you try to enable it, Cocoa UI won't compile at all because of many
errors like this one and similar ones:

../ui/cocoa.m:1186:12: error: ARC forbids explicit message send of
'dealloc'
    [super dealloc];
         ~~~~~ ^

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

> Introduction of Automatic Reference Counting would greatly help
> addressing those issues, but that would require significant rewriting
> of ui/cocoa.m.

Agreed.

Thanks,
Roman

P.S. I still think that given the mentioned facts and implicitness
introduced by Objective-C it would be more natural to have C code in
macOS-related device backends like vmnet and coreaudio unless
Objective-C is essential and required (like in UI code).

> Personally I'm concerned with ui/cocoa.m and do want to do that
> rewriting, but I'm being busy so it would not happen anytime soon.
> 
> Regards,
> Akihiko Odaki