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  wrote:
> >
> > On Mon, Jan 24, 2022 at 08:14:31PM +, Peter Maydell wrote:
> > > On Mon, 24 Jan 2022 at 17:49, Roman Bolshakov  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

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

[Akihiko Odaki]

On Fri, Jan 28, 2022 at 11:30 PM Vladislav Yaroshchuk
 wrote:
>
>
>
> пт, 21 янв. 2022 г. в 16:03, Akihiko Odaki :
>>
>> On Fri, Jan 21, 2022 at 9:19 PM Vladislav Yaroshchuk
>>  wrote:
>> >
>> >
>> > чт, 20 янв. 2022 г. в 11:32, Roman Bolshakov :
>> >>
>> >> On Thu, Jan 13, 2022 at 08:22:14PM +0300, Vladislav Yaroshchuk wrote:
>> >> > Create separate netdevs for each vmnet operating mode:
>> >> > - vmnet-host
>> >> > - vmnet-shared
>> >> > - vmnet-bridged
>> >> >
>> >>
>> >> Sure I'm late to the party but what if we add only one backend - vmnet
>> >> with default mode set to shared and all parameters are added there?
>> >>
>> >> The CLI would look more reasonable for the most typical use case:
>> >>  -netdev vmnet,id=if1 -device virtio-net,netdev=if1
>> >>
>> >> That would remove duplication of options in QAPI schema (e.g. isolated
>> >> is available in all backends now, altough I'm not sure if it makes sense
>> >> for bridged mode):
>> >>
>> >>  -netdev vmnet,id=if1,isolated=yes
>> >>
>> >> start-address, end-address and subnet-mask are also used by both shared
>> >> and host modes.
>> >>
>> >> Bridged netdev would lool like:
>> >>
>> >>  -netdev vmnet,id=if1,mode=bridged,ifname=en1
>> >>
>> >> Checksum offloading also seems to be available for all backends from
>> >> Monterey.
>> >>
>> >> The approach might simplify integration of the changes to libvirt and
>> >> discovery of upcoming vmnet features via qapi.
>> >>
>> >
>> > I can rewrite this if it sounds more suitable to use
>> > single `vmnet` netdev instead of three different ones.
>> > We can discuss this with Markus as a QAPI reviewer.
>> > Markus, what is your opinion about single netdev?
>> >
>> > P.S. Seems we have enough time for discussion:
>> > I started fixing memory leaks found by Akihiko and
>> > met a strange deadlock on QEMU shutdown on
>> > `qemu_mutex_lock_iothread()` during careful
>> > interface destruction with added semaphore.
>> > Need to go deeper to understand what's the
>> > problem, it will take some time.
>> >
>> > Mentioned part of Akihiko's review:
>> > https://patchew.org/QEMU/20220113172219.66372-1-yaroshchuk2...@gmail.com/20220113172219.66372-4-yaroshchuk2...@gmail.com/
>>
>
> > Actually I thought it would be tricky to implement.
>
> You mean single netdev type?

I meant the semaphore causing the deadlock, but I also think
implementing the single netdev type would have a difficulty because it
would need a redundant encapsulation of the option type as Phillip
Tennen's original patch series did.

>
>> Actually I thought it would be tricky to implement. A deadlock will
>> occur in a simple implementation if vmnet_send is already queued but
>> not executed yet when destructing:
>> - vmnet_send tries to lock the iothread and waits for the destructor to 
>> unlock.
>> - vmnet_stop_interface waits for vmnet_send finishing.
>>
>
> Sounds like that's what happens actually.
> With added semaphore:
> --- a/net/vmnet-common.m
> +++ b/net/vmnet-common.m
> @@ -320,13 +320,14 @@ void vmnet_cleanup_common(NetClientState *nc)
>  "org.qemu.vmnet.destroy",
>  DISPATCH_QUEUE_SERIAL
>  );
> -
> +dispatch_semaphore_t if_destroy_sem = dispatch_semaphore_create(0);
>  vmnet_stop_interface(
>  s->vmnet_if,
>  if_destroy_q,
>  ^(vmnet_return_t status) {
> +dispatch_semaphore_signal(if_destroy_sem);
>  });
> -
> +dispatch_semaphore_wait(if_destroy_sem, DISPATCH_TIME_FOREVER);
>  for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
>  g_free(s->iov_buf[i].iov_base);
>  }
> --
>
> I see this thread dump on deadlock:
>
> (lldb) bt all
> * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
>   * frame #0: 0x7fff2037ebb2 libsystem_kernel.dylib`__semwait_signal + 10
> frame #1: 0x7fff202fec1a libsystem_c.dylib`nanosleep + 196
> frame #2: 0x7fff212c4bb8 Foundation`+[NSThread sleepForTimeInterval:] 
> + 170
> frame #3: 0x000101ebce3a qemu-system-x86_64`-[QemuCocoaAppController 
> applicationWillTerminate:](self=0x7fa9f91a1de0, 
> _cmd="applicationWillTerminate:", 
> aNotification=@"NSApplicationWillTerminateNotification") at cocoa.m:1202:5
> frame #4: 0x7fff204a00cd 
> CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
> frame #5: 0x7fff2053bb1c 
> CoreFoundation`___CFXRegistrationPost_block_invoke + 49
> frame #6: 0x7fff2053ba9a CoreFoundation`_CFXRegistrationPost + 454
> frame #7: 0x7fff2047134e CoreFoundation`_CFXNotificationPost + 736
> frame #8: 0x7fff211e1bb8 Foundation`-[NSNotificationCenter 
> postNotificationName:object:userInfo:] + 59
> frame #9: 0x7fff22f585b3 AppKit`-[NSApplication terminate:] + 1377
> frame #10: 0x000101ebcf1f qemu-system-x86_64`-[QemuCocoaAppController 
> windowShouldClose:](self=0x7fa9f91a1de0, _cmd="windowShouldClose:", 
> sender=0x7fa9f91a7810) at cocoa.m:1231:5
> frame #11: 

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

[Vladislav Yaroshchuk]

пт, 21 янв. 2022 г. в 16:03, Akihiko Odaki :

> On Fri, Jan 21, 2022 at 9:19 PM Vladislav Yaroshchuk
>  wrote:
> >
> >
> > чт, 20 янв. 2022 г. в 11:32, Roman Bolshakov :
> >>
> >> On Thu, Jan 13, 2022 at 08:22:14PM +0300, Vladislav Yaroshchuk wrote:
> >> > Create separate netdevs for each vmnet operating mode:
> >> > - vmnet-host
> >> > - vmnet-shared
> >> > - vmnet-bridged
> >> >
> >>
> >> Sure I'm late to the party but what if we add only one backend - vmnet
> >> with default mode set to shared and all parameters are added there?
> >>
> >> The CLI would look more reasonable for the most typical use case:
> >>  -netdev vmnet,id=if1 -device virtio-net,netdev=if1
> >>
> >> That would remove duplication of options in QAPI schema (e.g. isolated
> >> is available in all backends now, altough I'm not sure if it makes sense
> >> for bridged mode):
> >>
> >>  -netdev vmnet,id=if1,isolated=yes
> >>
> >> start-address, end-address and subnet-mask are also used by both shared
> >> and host modes.
> >>
> >> Bridged netdev would lool like:
> >>
> >>  -netdev vmnet,id=if1,mode=bridged,ifname=en1
> >>
> >> Checksum offloading also seems to be available for all backends from
> >> Monterey.
> >>
> >> The approach might simplify integration of the changes to libvirt and
> >> discovery of upcoming vmnet features via qapi.
> >>
> >
> > I can rewrite this if it sounds more suitable to use
> > single `vmnet` netdev instead of three different ones.
> > We can discuss this with Markus as a QAPI reviewer.
> > Markus, what is your opinion about single netdev?
> >
> > P.S. Seems we have enough time for discussion:
> > I started fixing memory leaks found by Akihiko and
> > met a strange deadlock on QEMU shutdown on
> > `qemu_mutex_lock_iothread()` during careful
> > interface destruction with added semaphore.
> > Need to go deeper to understand what's the
> > problem, it will take some time.
> >
> > Mentioned part of Akihiko's review:
> >
> https://patchew.org/QEMU/20220113172219.66372-1-yaroshchuk2...@gmail.com/20220113172219.66372-4-yaroshchuk2...@gmail.com/
>
>
> Actually I thought it would be tricky to implement.

You mean single netdev type?

Actually I thought it would be tricky to implement. A deadlock will
> occur in a simple implementation if vmnet_send is already queued but
> not executed yet when destructing:
> - vmnet_send tries to lock the iothread and waits for the destructor to
> unlock.
> - vmnet_stop_interface waits for vmnet_send finishing.
>
>
Sounds like that's what happens actually.
With added semaphore:
--- a/net/vmnet-common.m
+++ b/net/vmnet-common.m
@@ -320,13 +320,14 @@ void vmnet_cleanup_common(NetClientState *nc)
 "org.qemu.vmnet.destroy",
 DISPATCH_QUEUE_SERIAL
 );
-
+dispatch_semaphore_t if_destroy_sem = dispatch_semaphore_create(0);
 vmnet_stop_interface(
 s->vmnet_if,
 if_destroy_q,
 ^(vmnet_return_t status) {
+dispatch_semaphore_signal(if_destroy_sem);
 });
-
+dispatch_semaphore_wait(if_destroy_sem, DISPATCH_TIME_FOREVER);
 for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
 g_free(s->iov_buf[i].iov_base);
 }
-- 

I see this thread dump on deadlock:

(lldb) bt all
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
  * frame #0: 0x7fff2037ebb2 libsystem_kernel.dylib`__semwait_signal +
10
frame #1: 0x7fff202fec1a libsystem_c.dylib`nanosleep + 196
frame #2: 0x7fff212c4bb8 Foundation`+[NSThread
sleepForTimeInterval:] + 170
frame #3: 0x000101ebce3a
qemu-system-x86_64`-[QemuCocoaAppController
applicationWillTerminate:](self=0x7fa9f91a1de0,
_cmd="applicationWillTerminate:",
aNotification=@"NSApplicationWillTerminateNotification") at cocoa.m:1202:5
frame #4: 0x7fff204a00cd
CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
frame #5: 0x7fff2053bb1c
CoreFoundation`___CFXRegistrationPost_block_invoke + 49
frame #6: 0x7fff2053ba9a CoreFoundation`_CFXRegistrationPost + 454
frame #7: 0x7fff2047134e CoreFoundation`_CFXNotificationPost + 736
frame #8: 0x7fff211e1bb8 Foundation`-[NSNotificationCenter
postNotificationName:object:userInfo:] + 59
frame #9: 0x7fff22f585b3 AppKit`-[NSApplication terminate:] + 1377
frame #10: 0x000101ebcf1f
qemu-system-x86_64`-[QemuCocoaAppController
windowShouldClose:](self=0x7fa9f91a1de0, _cmd="windowShouldClose:",
sender=0x7fa9f91a7810) at cocoa.m:1231:5
frame #11: 0x7fff230417c9 AppKit`__19-[NSWindow
__close]_block_invoke + 153
frame #12: 0x7fff23041723 AppKit`-[NSWindow __close] + 284
frame #13: 0x7fff22ed12bb AppKit`-[NSApplication(NSResponder)
sendAction:to:from:] + 288
frame #14: 0x7fff22ed115f AppKit`-[NSControl sendAction:to:] + 86
frame #15: 0x7fff22ed1091 AppKit`__26-[NSCell
_sendActionFrom:]_block_invoke + 131
frame #16: 0x7fff22ed0f98 AppKit`-[NSCell _sendActionFrom:] + 171
frame #17: 

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

[Christian Schoenebeck]

On Dienstag, 25. Januar 2022 12:08:21 CET Akihiko Odaki wrote:
> On Tue, Jan 25, 2022 at 7:32 PM Peter Maydell  
wrote:
> > On Tue, 25 Jan 2022 at 04:14, Akihiko Odaki  
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-carwynel...@gmail.com/
> > ("ui/cocoa: Add option to disable left command and hide cursor on click")
> > and
> > https://patchew.org/QEMU/20220103114515.24020-1-carwynel...@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

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

[Akihiko Odaki]

On Tue, Jan 25, 2022 at 7:32 PM Peter Maydell  wrote:
>
> On Tue, 25 Jan 2022 at 04:14, Akihiko Odaki  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-carwynel...@gmail.com/
> ("ui/cocoa: Add option to disable left command and hide cursor on click")
> and
> https://patchew.org/QEMU/20220103114515.24020-1-carwynel...@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

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

[Peter Maydell]

On Tue, 25 Jan 2022 at 04:14, Akihiko Odaki  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...)

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-carwynel...@gmail.com/
("ui/cocoa: Add option to disable left command and hide cursor on click")
and
https://patchew.org/QEMU/20220103114515.24020-1-carwynel...@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

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

[Akihiko Odaki]

On Tue, Jan 25, 2022 at 8:00 AM Roman Bolshakov  wrote:
>
> On Mon, Jan 24, 2022 at 08:14:31PM +, Peter Maydell wrote:
> > On Mon, 24 Jan 2022 at 17:49, Roman Bolshakov  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.
>
> Thanks,
> Roman

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.

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). Introduction of
Automatic Reference Counting would greatly help addressing those
issues, but that would require significant rewriting of ui/cocoa.m.
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

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

[Roman Bolshakov]

On Mon, Jan 24, 2022 at 08:14:31PM +, Peter Maydell wrote:
> On Mon, 24 Jan 2022 at 17:49, Roman Bolshakov  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.

Thanks,
Roman

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

[Christian Schoenebeck]

On Montag, 24. Januar 2022 18:49:53 CET Roman Bolshakov wrote:
> On Mon, Jan 24, 2022 at 12:27:40PM +0100, Christian Schoenebeck wrote:
> > On Montag, 24. Januar 2022 10:56:00 CET Roman Bolshakov wrote:
> > > On Thu, Jan 13, 2022 at 08:22:14PM +0300, Vladislav Yaroshchuk wrote:
> > > >  net/vmnet-bridged.m |  25 +
> > > >  net/vmnet-common.m  |  20 +++
> > > 
> > > It seems the last two files should have .c extension rather than .m.
> > 
> > I would not do that. Mind cross-compilers, please.
> 
> Hi Christian,
> 
> Cross-compilers for Apple platforms can be constructed using à la carte
> approach where toolchain comes from the source, SDK from Apple and a
> port of cctools from GitHub (mind all library dependencies of QEMU).
> That's quite an effort!
> 
> I very much doubt this is a relevant and typical case for QEMU on macOS.
> And if cross-compiler is constructed properly it'll pass required flags
> that enable blocks and will link block runtime in its default build
> recipe like all cross-compilers do for the platform of interest.
> 
> Gladly, there's osxcross [1] and crossbuild image with Darwin support [2].
> They can deal with blocks just fine:
> 
>   # CROSS_TRIPLE=i386-apple-darwin
>   $ cc block.c && file a.out
>   a.out: Mach-O i386 executable,
> flags:
> 
>   # CROSS_TRIPLE=x86_64-apple-darwin
>   $ cc block.c && file a.out
>   $ file a.out
>   a.out: Mach-O 64-bit x86_64 executable,
> flags:
> > > Unlike Cocoa UI code, the files do not contain Objective-C classes. They
> > > are just C code with blocks (which is supported by compilers shipped
> > > with Xcode SDK), e.g this program can be compiled without extra
> > > compiler flags:
> > > 
> > > $ cat block.c
> > > int main() {
> > > 
> > > int (^x)(void) = ^{
> > > 
> > > return 0;
> > > 
> > > };
> > > 
> > > return x();
> > > 
> > > }
> > > $ cc block.c && ./a.out
> > > $
> > 
> > Such blocks are still Objective-C language specific, they are not C and
> > therefore won't work with GCC.
> 
> 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.

Because blocks are automatically available if you are using an Objective-C or 
Objective-C++ frontend, but not necessarily if you use a C or C++ frontend.

> > $ gcc block.c
> > 
> > block.c: In function ‘main’:
> > block.c:2:14: error: expected identifier or ‘(’ before ‘^’ token
> > 
> >  int (^x)(void) = ^{
> >  
> >   ^
> > 
> > block.c:6:16: warning: implicit declaration of function ‘x’ [-Wimplicit-
> > function-declaration]
> > 
> >  return x();
> >  
> > ^
> 
> You might do this on Linux and it'll work:
> 
> $ clang -g -fblocks -lBlocksRuntime block.c && ./a.out

Yes, which is an unnecesary complicated & limiting variant of just:

clang/gcc block.m

Don't get me wrong, I don't care too much about this issue. It's just that I 
really see no advantage in renaming this into a C file, but I do see 
disadvantages. That's all.

> However, vmnet code won't be compiled on non-Apple platforms because the
> compilation happens only if vmnet is available which happens only if
> appleframeworks dependency is available, that is not available on
> non-OSX hosts [6]:
> 
>   "These dependencies can never be found for non-OSX hosts."
> 
> 1. https://github.com/tpoechtrager/osxcross
> 2. https://github.com/multiarch/crossbuild
> 3. http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1370.pdf
> 4. https://clang.llvm.org/docs/BlockLanguageSpec.html
> 5. https://clang.llvm.org/docs/Block-ABI-Apple.html
> 6. https://mesonbuild.com/Dependencies.html#appleframeworks
> 
> Regards,
> Roman

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

[Peter Maydell]

On Mon, 24 Jan 2022 at 17:49, Roman Bolshakov  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.

thanks
-- PMM

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

[Roman Bolshakov]

On Mon, Jan 24, 2022 at 12:27:40PM +0100, Christian Schoenebeck wrote:
> On Montag, 24. Januar 2022 10:56:00 CET Roman Bolshakov wrote:
> > On Thu, Jan 13, 2022 at 08:22:14PM +0300, Vladislav Yaroshchuk wrote:
> > >  net/vmnet-bridged.m |  25 +
> > >  net/vmnet-common.m  |  20 +++
> > 
> > It seems the last two files should have .c extension rather than .m.
> 
> I would not do that. Mind cross-compilers, please.
> 

Hi Christian,

Cross-compilers for Apple platforms can be constructed using à la carte
approach where toolchain comes from the source, SDK from Apple and a
port of cctools from GitHub (mind all library dependencies of QEMU).
That's quite an effort!

I very much doubt this is a relevant and typical case for QEMU on macOS.
And if cross-compiler is constructed properly it'll pass required flags
that enable blocks and will link block runtime in its default build
recipe like all cross-compilers do for the platform of interest.

Gladly, there's osxcross [1] and crossbuild image with Darwin support [2].
They can deal with blocks just fine:

  # CROSS_TRIPLE=i386-apple-darwin
  $ cc block.c && file a.out
  a.out: Mach-O i386 executable, 
flags:

  # CROSS_TRIPLE=x86_64-apple-darwin
  $ cc block.c && file a.out
  $ file a.out
  a.out: Mach-O 64-bit x86_64 executable, flags:

> > Unlike Cocoa UI code, the files do not contain Objective-C classes. They are
> > just C code with blocks (which is supported by compilers shipped with Xcode
> > SDK), e.g this program can be compiled without extra compiler flags:
> > 
> > $ cat block.c
> > int main() {
> > int (^x)(void) = ^{
> > return 0;
> > };
> > 
> > return x();
> > }
> > $ cc block.c && ./a.out
> > $
> > 
> 
> Such blocks are still Objective-C language specific, they are not C and 
> therefore won't work with GCC.
> 

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.

> $ gcc block.c
> 
> block.c: In function ‘main’:
> block.c:2:14: error: expected identifier or ‘(’ before ‘^’ token
>  int (^x)(void) = ^{
>   ^
> block.c:6:16: warning: implicit declaration of function ‘x’ [-Wimplicit-
> function-declaration]
>  return x();
> ^

You might do this on Linux and it'll work:

$ clang -g -fblocks -lBlocksRuntime block.c && ./a.out

However, vmnet code won't be compiled on non-Apple platforms because the
compilation happens only if vmnet is available which happens only if
appleframeworks dependency is available, that is not available on
non-OSX hosts [6]:

  "These dependencies can never be found for non-OSX hosts."

1. https://github.com/tpoechtrager/osxcross
2. https://github.com/multiarch/crossbuild
3. http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1370.pdf
4. https://clang.llvm.org/docs/BlockLanguageSpec.html
5. https://clang.llvm.org/docs/Block-ABI-Apple.html
6. https://mesonbuild.com/Dependencies.html#appleframeworks

Regards,
Roman

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

[Christian Schoenebeck]

On Montag, 24. Januar 2022 10:56:00 CET Roman Bolshakov wrote:
> On Thu, Jan 13, 2022 at 08:22:14PM +0300, Vladislav Yaroshchuk wrote:
> > Create separate netdevs for each vmnet operating mode:
> > - vmnet-host
> > - vmnet-shared
> > - vmnet-bridged
> > 
> > Signed-off-by: Vladislav Yaroshchuk 
> > ---
> > 
> >  net/clients.h   |  11 
> >  net/meson.build |   7 +++
> >  net/net.c   |  10 
> >  net/vmnet-bridged.m |  25 +
> >  net/vmnet-common.m  |  20 +++
> 
> Hi Vladislav,
> 
> It seems the last two files should have .c extension rather than .m.

I would not do that. Mind cross-compilers, please.

> Unlike Cocoa UI code, the files do not contain Objective-C classes. They are
> just C code with blocks (which is supported by compilers shipped with Xcode
> SDK), e.g this program can be compiled without extra compiler flags:
> 
> $ cat block.c
> int main() {
> int (^x)(void) = ^{
> return 0;
> };
> 
> return x();
> }
> $ cc block.c && ./a.out
> $
> 
> Regards,
> Roman

Such blocks are still Objective-C language specific, they are not C and 
therefore won't work with GCC.

$ gcc block.c

block.c: In function ‘main’:
block.c:2:14: error: expected identifier or ‘(’ before ‘^’ token
 int (^x)(void) = ^{
  ^
block.c:6:16: warning: implicit declaration of function ‘x’ [-Wimplicit-
function-declaration]
 return x();
^
Best regards,
Christian Schoenebeck

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

[Roman Bolshakov]

On Thu, Jan 13, 2022 at 08:22:14PM +0300, Vladislav Yaroshchuk wrote:
> Create separate netdevs for each vmnet operating mode:
> - vmnet-host
> - vmnet-shared
> - vmnet-bridged
> 
> Signed-off-by: Vladislav Yaroshchuk 
> ---
>  net/clients.h   |  11 
>  net/meson.build |   7 +++
>  net/net.c   |  10 
>  net/vmnet-bridged.m |  25 +
>  net/vmnet-common.m  |  20 +++

Hi Vladislav,

It seems the last two files should have .c extension rather than .m.

Unlike Cocoa UI code, the files do not contain Objective-C classes. They are
just C code with blocks (which is supported by compilers shipped with Xcode
SDK), e.g this program can be compiled without extra compiler flags:

$ cat block.c
int main() {
int (^x)(void) = ^{
return 0;
};

return x();
}
$ cc block.c && ./a.out
$

Regards,
Roman

>  net/vmnet-host.c|  24 
>  net/vmnet-shared.c  |  25 +
>  net/vmnet_int.h |  25 +
>  qapi/net.json   | 133 +++-
>  9 files changed, 278 insertions(+), 2 deletions(-)
>  create mode 100644 net/vmnet-bridged.m
>  create mode 100644 net/vmnet-common.m
>  create mode 100644 net/vmnet-host.c
>  create mode 100644 net/vmnet-shared.c
>  create mode 100644 net/vmnet_int.h
> 
> diff --git a/net/clients.h b/net/clients.h
> index 92f9b59aed..c9157789f2 100644
> --- a/net/clients.h
> +++ b/net/clients.h
> @@ -63,4 +63,15 @@ int net_init_vhost_user(const Netdev *netdev, const char 
> *name,
>  
>  int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>  NetClientState *peer, Error **errp);
> +#ifdef CONFIG_VMNET
> +int net_init_vmnet_host(const Netdev *netdev, const char *name,
> +  NetClientState *peer, Error **errp);
> +
> +int net_init_vmnet_shared(const Netdev *netdev, const char *name,
> +  NetClientState *peer, Error **errp);
> +
> +int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
> +  NetClientState *peer, Error **errp);
> +#endif /* CONFIG_VMNET */
> +
>  #endif /* QEMU_NET_CLIENTS_H */
> diff --git a/net/meson.build b/net/meson.build
> index 847bc2ac85..00a88c4951 100644
> --- a/net/meson.build
> +++ b/net/meson.build
> @@ -42,4 +42,11 @@ softmmu_ss.add(when: 'CONFIG_POSIX', if_true: 
> files(tap_posix))
>  softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('tap-win32.c'))
>  softmmu_ss.add(when: 'CONFIG_VHOST_NET_VDPA', if_true: files('vhost-vdpa.c'))
>  
> +vmnet_files = files(
> +  'vmnet-common.m',
> +  'vmnet-bridged.m',
> +  'vmnet-host.c',
> +  'vmnet-shared.c'
> +)
> +softmmu_ss.add(when: vmnet, if_true: vmnet_files)
>  subdir('can')
> diff --git a/net/net.c b/net/net.c
> index f0d14dbfc1..1dbb64b935 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1021,6 +1021,11 @@ static int (* const 
> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>  #ifdef CONFIG_L2TPV3
>  [NET_CLIENT_DRIVER_L2TPV3]= net_init_l2tpv3,
>  #endif
> +#ifdef CONFIG_VMNET
> +[NET_CLIENT_DRIVER_VMNET_HOST] = net_init_vmnet_host,
> +[NET_CLIENT_DRIVER_VMNET_SHARED] = net_init_vmnet_shared,
> +[NET_CLIENT_DRIVER_VMNET_BRIDGED] = net_init_vmnet_bridged,
> +#endif /* CONFIG_VMNET */
>  };
>  
>  
> @@ -1106,6 +,11 @@ void show_netdevs(void)
>  #endif
>  #ifdef CONFIG_VHOST_VDPA
>  "vhost-vdpa",
> +#endif
> +#ifdef CONFIG_VMNET
> +"vmnet-host",
> +"vmnet-shared",
> +"vmnet-bridged",
>  #endif
>  };
>  
> diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
> new file mode 100644
> index 00..4e42a90391
> --- /dev/null
> +++ b/net/vmnet-bridged.m
> @@ -0,0 +1,25 @@
> +/*
> + * vmnet-bridged.m
> + *
> + * Copyright(c) 2021 Vladislav Yaroshchuk 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-types-net.h"
> +#include "vmnet_int.h"
> +#include "clients.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +
> +#include 
> +
> +int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
> +   NetClientState *peer, Error **errp)
> +{
> +  error_setg(errp, "vmnet-bridged is not implemented yet");
> +  return -1;
> +}
> diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> new file mode 100644
> index 00..532d152840
> --- /dev/null
> +++ b/net/vmnet-common.m
> @@ -0,0 +1,20 @@
> +/*
> + * vmnet-common.m - network client wrapper for Apple vmnet.framework
> + *
> + * Copyright(c) 2021 Vladislav Yaroshchuk 
> + * Copyright(c) 2021 Phillip Tennen 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-types-net.h"
> +#include "vmnet_int.h"
> +#include 

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

[Akihiko Odaki]

On Fri, Jan 21, 2022 at 9:19 PM Vladislav Yaroshchuk
 wrote:
>
>
> чт, 20 янв. 2022 г. в 11:32, Roman Bolshakov :
>>
>> On Thu, Jan 13, 2022 at 08:22:14PM +0300, Vladislav Yaroshchuk wrote:
>> > Create separate netdevs for each vmnet operating mode:
>> > - vmnet-host
>> > - vmnet-shared
>> > - vmnet-bridged
>> >
>>
>> Sure I'm late to the party but what if we add only one backend - vmnet
>> with default mode set to shared and all parameters are added there?
>>
>> The CLI would look more reasonable for the most typical use case:
>>  -netdev vmnet,id=if1 -device virtio-net,netdev=if1
>>
>> That would remove duplication of options in QAPI schema (e.g. isolated
>> is available in all backends now, altough I'm not sure if it makes sense
>> for bridged mode):
>>
>>  -netdev vmnet,id=if1,isolated=yes
>>
>> start-address, end-address and subnet-mask are also used by both shared
>> and host modes.
>>
>> Bridged netdev would lool like:
>>
>>  -netdev vmnet,id=if1,mode=bridged,ifname=en1
>>
>> Checksum offloading also seems to be available for all backends from
>> Monterey.
>>
>> The approach might simplify integration of the changes to libvirt and
>> discovery of upcoming vmnet features via qapi.
>>
>
> I can rewrite this if it sounds more suitable to use
> single `vmnet` netdev instead of three different ones.
> We can discuss this with Markus as a QAPI reviewer.
> Markus, what is your opinion about single netdev?
>
> P.S. Seems we have enough time for discussion:
> I started fixing memory leaks found by Akihiko and
> met a strange deadlock on QEMU shutdown on
> `qemu_mutex_lock_iothread()` during careful
> interface destruction with added semaphore.
> Need to go deeper to understand what's the
> problem, it will take some time.
>
> Mentioned part of Akihiko's review:
> https://patchew.org/QEMU/20220113172219.66372-1-yaroshchuk2...@gmail.com/20220113172219.66372-4-yaroshchuk2...@gmail.com/

Actually I thought it would be tricky to implement. A deadlock will
occur in a simple implementation if vmnet_send is already queued but
not executed yet when destructing:
- vmnet_send tries to lock the iothread and waits for the destructor to unlock.
- vmnet_stop_interface waits for vmnet_send finishing.

Though I doubt it is the cause of your deadlock. This deadlock would
not happen frequently since vmnet_send will not be queued if the
device is not used.

Regards,
Akihiko Odaki

>
>
>> Thanks,
>> Roman
>>
>> > Signed-off-by: Vladislav Yaroshchuk 
>> > ---
>> >  net/clients.h   |  11 
>> >  net/meson.build |   7 +++
>> >  net/net.c   |  10 
>> >  net/vmnet-bridged.m |  25 +
>> >  net/vmnet-common.m  |  20 +++
>> >  net/vmnet-host.c|  24 
>> >  net/vmnet-shared.c  |  25 +
>> >  net/vmnet_int.h |  25 +
>> >  qapi/net.json   | 133 +++-
>> >  9 files changed, 278 insertions(+), 2 deletions(-)
>> >  create mode 100644 net/vmnet-bridged.m
>> >  create mode 100644 net/vmnet-common.m
>> >  create mode 100644 net/vmnet-host.c
>> >  create mode 100644 net/vmnet-shared.c
>> >  create mode 100644 net/vmnet_int.h
>> >
>> > diff --git a/net/net.c b/net/net.c
>> > index f0d14dbfc1..1dbb64b935 100644
>> > --- a/net/net.c
>> > +++ b/net/net.c
>> > @@ -1021,6 +1021,11 @@ static int (* const 
>> > net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>> >  #ifdef CONFIG_L2TPV3
>> >  [NET_CLIENT_DRIVER_L2TPV3]= net_init_l2tpv3,
>> >  #endif
>> > +#ifdef CONFIG_VMNET
>> > +[NET_CLIENT_DRIVER_VMNET_HOST] = net_init_vmnet_host,
>> > +[NET_CLIENT_DRIVER_VMNET_SHARED] = net_init_vmnet_shared,
>> > +[NET_CLIENT_DRIVER_VMNET_BRIDGED] = net_init_vmnet_bridged,
>> > +#endif /* CONFIG_VMNET */
>> >  };
>> >
>> >
>> > @@ -1106,6 +,11 @@ void show_netdevs(void)
>> >  #endif
>> >  #ifdef CONFIG_VHOST_VDPA
>> >  "vhost-vdpa",
>> > +#endif
>> > +#ifdef CONFIG_VMNET
>> > +"vmnet-host",
>> > +"vmnet-shared",
>> > +"vmnet-bridged",
>> >  #endif
>> >  };
>> >
>> > diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
>> > new file mode 100644
>> > index 00..4e42a90391
>> > --- /dev/null
>> > +++ b/net/vmnet-bridged.m
>> > @@ -0,0 +1,25 @@
>> > +/*
>> > + * vmnet-bridged.m
>> > + *
>> > + * Copyright(c) 2021 Vladislav Yaroshchuk 
>> > + *
>> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
>> > later.
>> > + * See the COPYING file in the top-level directory.
>> > + *
>> > + */
>> > +
>> > +#include "qemu/osdep.h"
>> > +#include "qapi/qapi-types-net.h"
>> > +#include "vmnet_int.h"
>> > +#include "clients.h"
>> > +#include "qemu/error-report.h"
>> > +#include "qapi/error.h"
>> > +
>> > +#include 
>> > +
>> > +int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
>> > +   NetClientState *peer, Error **errp)
>> > +{
>> > +  error_setg(errp, "vmnet-bridged is not implemented yet");
>> > +  return -1;
>> > +}
>> > diff --git 

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

[Vladislav Yaroshchuk]

чт, 20 янв. 2022 г. в 11:32, Roman Bolshakov :

> On Thu, Jan 13, 2022 at 08:22:14PM +0300, Vladislav Yaroshchuk wrote:
> > Create separate netdevs for each vmnet operating mode:
> > - vmnet-host
> > - vmnet-shared
> > - vmnet-bridged
> >
>
> Sure I'm late to the party but what if we add only one backend - vmnet
> with default mode set to shared and all parameters are added there?
>
> The CLI would look more reasonable for the most typical use case:
>  -netdev vmnet,id=if1 -device virtio-net,netdev=if1
>
> That would remove duplication of options in QAPI schema (e.g. isolated
> is available in all backends now, altough I'm not sure if it makes sense
> for bridged mode):
>
>  -netdev vmnet,id=if1,isolated=yes
>
> start-address, end-address and subnet-mask are also used by both shared
> and host modes.
>
> Bridged netdev would lool like:
>
>  -netdev vmnet,id=if1,mode=bridged,ifname=en1
>
> Checksum offloading also seems to be available for all backends from
> Monterey.
>
> The approach might simplify integration of the changes to libvirt and
> discovery of upcoming vmnet features via qapi.
>
>
I can rewrite this if it sounds more suitable to use
single `vmnet` netdev instead of three different ones.
We can discuss this with Markus as a QAPI reviewer.
Markus, what is your opinion about single netdev?

P.S. Seems we have enough time for discussion:
I started fixing memory leaks found by Akihiko and
met a strange deadlock on QEMU shutdown on
`qemu_mutex_lock_iothread()` during careful
interface destruction with added semaphore.
Need to go deeper to understand what's the
problem, it will take some time.

Mentioned part of Akihiko's review:
https://patchew.org/QEMU/20220113172219.66372-1-yaroshchuk2...@gmail.com/20220113172219.66372-4-yaroshchuk2...@gmail.com/


Thanks,
> Roman
>
> > Signed-off-by: Vladislav Yaroshchuk 
> > ---
> >  net/clients.h   |  11 
> >  net/meson.build |   7 +++
> >  net/net.c   |  10 
> >  net/vmnet-bridged.m |  25 +
> >  net/vmnet-common.m  |  20 +++
> >  net/vmnet-host.c|  24 
> >  net/vmnet-shared.c  |  25 +
> >  net/vmnet_int.h |  25 +
> >  qapi/net.json   | 133 +++-
> >  9 files changed, 278 insertions(+), 2 deletions(-)
> >  create mode 100644 net/vmnet-bridged.m
> >  create mode 100644 net/vmnet-common.m
> >  create mode 100644 net/vmnet-host.c
> >  create mode 100644 net/vmnet-shared.c
> >  create mode 100644 net/vmnet_int.h
> >
> > diff --git a/net/net.c b/net/net.c
> > index f0d14dbfc1..1dbb64b935 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1021,6 +1021,11 @@ static int (* const
> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
> >  #ifdef CONFIG_L2TPV3
> >  [NET_CLIENT_DRIVER_L2TPV3]= net_init_l2tpv3,
> >  #endif
> > +#ifdef CONFIG_VMNET
> > +[NET_CLIENT_DRIVER_VMNET_HOST] = net_init_vmnet_host,
> > +[NET_CLIENT_DRIVER_VMNET_SHARED] = net_init_vmnet_shared,
> > +[NET_CLIENT_DRIVER_VMNET_BRIDGED] = net_init_vmnet_bridged,
> > +#endif /* CONFIG_VMNET */
> >  };
> >
> >
> > @@ -1106,6 +,11 @@ void show_netdevs(void)
> >  #endif
> >  #ifdef CONFIG_VHOST_VDPA
> >  "vhost-vdpa",
> > +#endif
> > +#ifdef CONFIG_VMNET
> > +"vmnet-host",
> > +"vmnet-shared",
> > +"vmnet-bridged",
> >  #endif
> >  };
> >
> > diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
> > new file mode 100644
> > index 00..4e42a90391
> > --- /dev/null
> > +++ b/net/vmnet-bridged.m
> > @@ -0,0 +1,25 @@
> > +/*
> > + * vmnet-bridged.m
> > + *
> > + * Copyright(c) 2021 Vladislav Yaroshchuk 
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/qapi-types-net.h"
> > +#include "vmnet_int.h"
> > +#include "clients.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/error.h"
> > +
> > +#include 
> > +
> > +int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
> > +   NetClientState *peer, Error **errp)
> > +{
> > +  error_setg(errp, "vmnet-bridged is not implemented yet");
> > +  return -1;
> > +}
> > diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> > new file mode 100644
> > index 00..532d152840
> > --- /dev/null
> > +++ b/net/vmnet-common.m
> > @@ -0,0 +1,20 @@
> > +/*
> > + * vmnet-common.m - network client wrapper for Apple vmnet.framework
> > + *
> > + * Copyright(c) 2021 Vladislav Yaroshchuk 
> > + * Copyright(c) 2021 Phillip Tennen 
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/qapi-types-net.h"
> > +#include "vmnet_int.h"
> > +#include "clients.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/error.h"
> > 

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

[Roman Bolshakov]

On Thu, Jan 13, 2022 at 08:22:14PM +0300, Vladislav Yaroshchuk wrote:
> Create separate netdevs for each vmnet operating mode:
> - vmnet-host
> - vmnet-shared
> - vmnet-bridged
> 

Sure I'm late to the party but what if we add only one backend - vmnet
with default mode set to shared and all parameters are added there?

The CLI would look more reasonable for the most typical use case:
 -netdev vmnet,id=if1 -device virtio-net,netdev=if1

That would remove duplication of options in QAPI schema (e.g. isolated
is available in all backends now, altough I'm not sure if it makes sense
for bridged mode):

 -netdev vmnet,id=if1,isolated=yes

start-address, end-address and subnet-mask are also used by both shared
and host modes.

Bridged netdev would lool like:

 -netdev vmnet,id=if1,mode=bridged,ifname=en1

Checksum offloading also seems to be available for all backends from
Monterey.

The approach might simplify integration of the changes to libvirt and
discovery of upcoming vmnet features via qapi.

Thanks,
Roman

> Signed-off-by: Vladislav Yaroshchuk 
> ---
>  net/clients.h   |  11 
>  net/meson.build |   7 +++
>  net/net.c   |  10 
>  net/vmnet-bridged.m |  25 +
>  net/vmnet-common.m  |  20 +++
>  net/vmnet-host.c|  24 
>  net/vmnet-shared.c  |  25 +
>  net/vmnet_int.h |  25 +
>  qapi/net.json   | 133 +++-
>  9 files changed, 278 insertions(+), 2 deletions(-)
>  create mode 100644 net/vmnet-bridged.m
>  create mode 100644 net/vmnet-common.m
>  create mode 100644 net/vmnet-host.c
>  create mode 100644 net/vmnet-shared.c
>  create mode 100644 net/vmnet_int.h
> 
> diff --git a/net/net.c b/net/net.c
> index f0d14dbfc1..1dbb64b935 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1021,6 +1021,11 @@ static int (* const 
> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>  #ifdef CONFIG_L2TPV3
>  [NET_CLIENT_DRIVER_L2TPV3]= net_init_l2tpv3,
>  #endif
> +#ifdef CONFIG_VMNET
> +[NET_CLIENT_DRIVER_VMNET_HOST] = net_init_vmnet_host,
> +[NET_CLIENT_DRIVER_VMNET_SHARED] = net_init_vmnet_shared,
> +[NET_CLIENT_DRIVER_VMNET_BRIDGED] = net_init_vmnet_bridged,
> +#endif /* CONFIG_VMNET */
>  };
>  
>  
> @@ -1106,6 +,11 @@ void show_netdevs(void)
>  #endif
>  #ifdef CONFIG_VHOST_VDPA
>  "vhost-vdpa",
> +#endif
> +#ifdef CONFIG_VMNET
> +"vmnet-host",
> +"vmnet-shared",
> +"vmnet-bridged",
>  #endif
>  };
>  
> diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
> new file mode 100644
> index 00..4e42a90391
> --- /dev/null
> +++ b/net/vmnet-bridged.m
> @@ -0,0 +1,25 @@
> +/*
> + * vmnet-bridged.m
> + *
> + * Copyright(c) 2021 Vladislav Yaroshchuk 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-types-net.h"
> +#include "vmnet_int.h"
> +#include "clients.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +
> +#include 
> +
> +int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
> +   NetClientState *peer, Error **errp)
> +{
> +  error_setg(errp, "vmnet-bridged is not implemented yet");
> +  return -1;
> +}
> diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> new file mode 100644
> index 00..532d152840
> --- /dev/null
> +++ b/net/vmnet-common.m
> @@ -0,0 +1,20 @@
> +/*
> + * vmnet-common.m - network client wrapper for Apple vmnet.framework
> + *
> + * Copyright(c) 2021 Vladislav Yaroshchuk 
> + * Copyright(c) 2021 Phillip Tennen 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-types-net.h"
> +#include "vmnet_int.h"
> +#include "clients.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +
> +#include 
> +
> diff --git a/net/vmnet-host.c b/net/vmnet-host.c
> new file mode 100644
> index 00..4a5ef99dc7
> --- /dev/null
> +++ b/net/vmnet-host.c
> @@ -0,0 +1,24 @@
> +/*
> + * vmnet-host.c
> + *
> + * Copyright(c) 2021 Vladislav Yaroshchuk 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-types-net.h"
> +#include "vmnet_int.h"
> +#include "clients.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +
> +#include 
> +
> +int net_init_vmnet_host(const Netdev *netdev, const char *name,
> +NetClientState *peer, Error **errp) {
> +  error_setg(errp, "vmnet-host is not implemented yet");
> +  return -1;
> +}
> diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
> new file mode 100644
> index 00..f8c4a4f3b8
> --- /dev/null
> +++ b/net/vmnet-shared.c
> 

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

[Vladislav Yaroshchuk]

вт, 18 янв. 2022 г. в 19:35, Markus Armbruster :

> Vladislav Yaroshchuk  writes:
>
> > вт, 18 янв. 2022 г. в 18:01, Markus Armbruster :
> >
> >> Vladislav Yaroshchuk  writes:
> >>
> >> > Create separate netdevs for each vmnet operating mode:
> >> > - vmnet-host
> >> > - vmnet-shared
> >> > - vmnet-bridged
> >> >
> >> > Signed-off-by: Vladislav Yaroshchuk 
> >>
> >> I acked v8 of the QAPI schema part.  You should add Acked-by and
> >> Reviewed-by you receive in later revisions, unless you make changes that
> >> invalidate them.  When in doubt, drop them.
> >>
> >>
> > Oh ok, I'll do that next time.
>
> Thanks :)
>
> >> > diff --git a/qapi/net.json b/qapi/net.json
> >> > index 7fab2e7cd8..b922e2e34f 100644
> >> > --- a/qapi/net.json
> >> > +++ b/qapi/net.json
> >> > @@ -452,6 +452,120 @@
> >> >  '*vhostdev': 'str',
> >> >  '*queues':   'int' } }
> >> >
> >> > +##
> >> > +# @NetdevVmnetHostOptions:
> >> > +#
> >> > +# vmnet (host mode) network backend.
> >> > +#
> >> > +# Allows the vmnet interface to communicate with other vmnet
> >> > +# interfaces that are in host mode and also with the host.
> >> > +#
> >> > +# @start-address: The starting IPv4 address to use for the interface.
> >> > +# Must be in the private IP range (RFC 1918). Must be
> >> > +# specified along with @end-address and @subnet-mask.
> >> > +# This address is used as the gateway address. The
> >> > +# subsequent address up to and including end-address
> are
> >> > +# placed in the DHCP pool.
> >> > +#
> >> > +# @end-address: The DHCP IPv4 range end address to use for the
> >> > +#   interface. Must be in the private IP range (RFC
> 1918).
> >> > +#   Must be specified along with @start-address and
> >> > +#   @subnet-mask.
> >> > +#
> >> > +# @subnet-mask: The IPv4 subnet mask to use on the interface. Must
> >> > +#   be specified along with @start-address and
> @subnet-mask.
> >> > +#
> >> > +# @isolated: Enable isolation for this interface. Interface isolation
> >> > +#ensures that vmnet interface is not able to communicate
> >> > +#with any other vmnet interfaces. Only communication with
> >> > +#host is allowed. Available since macOS Big Sur 11.0.
> >>
> >> What happens when the host is too old?
> >>
> >>
> > In this case netdev creation will fail with
> > corresponding message (error_setg() used).
>
> "Available" feels slightly misleading.  It's always available, it just
> doesn't work unless the host OS is new enough.  Suggest something like
> "Requires at least macOS Big Sur 11.0."
>
>
Yep, "Requires" sounds much more suitable. Will update
the description in the next version along with other fixes.

Thank you!

Same for the others.
>
> QAPI schema
> Acked-by: Markus Armbruster 
>
> [...]
>
>

-- 
Best Regards,

Vladislav Yaroshchuk

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

[Vladislav Yaroshchuk]

вт, 18 янв. 2022 г. в 18:01, Markus Armbruster :

> Vladislav Yaroshchuk  writes:
>
> > Create separate netdevs for each vmnet operating mode:
> > - vmnet-host
> > - vmnet-shared
> > - vmnet-bridged
> >
> > Signed-off-by: Vladislav Yaroshchuk 
>
> I acked v8 of the QAPI schema part.  You should add Acked-by and
> Reviewed-by you receive in later revisions, unless you make changes that
> invalidate them.  When in doubt, drop them.
>
>
Oh ok, I'll do that next time.


> > diff --git a/qapi/net.json b/qapi/net.json
> > index 7fab2e7cd8..b922e2e34f 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -452,6 +452,120 @@
> >  '*vhostdev': 'str',
> >  '*queues':   'int' } }
> >
> > +##
> > +# @NetdevVmnetHostOptions:
> > +#
> > +# vmnet (host mode) network backend.
> > +#
> > +# Allows the vmnet interface to communicate with other vmnet
> > +# interfaces that are in host mode and also with the host.
> > +#
> > +# @start-address: The starting IPv4 address to use for the interface.
> > +# Must be in the private IP range (RFC 1918). Must be
> > +# specified along with @end-address and @subnet-mask.
> > +# This address is used as the gateway address. The
> > +# subsequent address up to and including end-address are
> > +# placed in the DHCP pool.
> > +#
> > +# @end-address: The DHCP IPv4 range end address to use for the
> > +#   interface. Must be in the private IP range (RFC 1918).
> > +#   Must be specified along with @start-address and
> > +#   @subnet-mask.
> > +#
> > +# @subnet-mask: The IPv4 subnet mask to use on the interface. Must
> > +#   be specified along with @start-address and @subnet-mask.
> > +#
> > +# @isolated: Enable isolation for this interface. Interface isolation
> > +#ensures that vmnet interface is not able to communicate
> > +#with any other vmnet interfaces. Only communication with
> > +#host is allowed. Available since macOS Big Sur 11.0.
>
> What happens when the host is too old?
>
>
In this case netdev creation will fail with
corresponding message (error_setg() used).

> +#
> > +# @net-uuid: The identifier (UUID) to uniquely identify the isolated
> > +#network vmnet interface should be added to. If
> > +#set, no DHCP service is provided for this interface and
> > +#network communication is allowed only with other interfaces
> > +#added to this network identified by the UUID. Available
> > +#since macOS Big Sur 11.0.
>
> Same question.
>
>
The same behavior here.


> > +#
> > +# Since: 7.0
> > +##
> > +{ 'struct': 'NetdevVmnetHostOptions',
> > +  'data': {
> > +'*start-address': 'str',
> > +'*end-address':   'str',
> > +'*subnet-mask':   'str',
> > +'*isolated':  'bool',
> > +'*net-uuid':  'str' },
> > +  'if': 'CONFIG_VMNET' }
> > +
> > +##
> > +# @NetdevVmnetSharedOptions:
> > +#
> > +# vmnet (shared mode) network backend.
> > +#
> > +# Allows traffic originating from the vmnet interface to reach the
> > +# Internet through a network address translator (NAT).
> > +# The vmnet interface can communicate with the host and with
> > +# other shared mode interfaces on the same subnet. If no DHCP
> > +# settings, subnet mask and IPv6 prefix specified, the interface can
> > +# communicate with any of other interfaces in shared mode.
> > +#
> > +# @start-address: The starting IPv4 address to use for the interface.
> > +# Must be in the private IP range (RFC 1918). Must be
> > +# specified along with @end-address and @subnet-mask.
> > +# This address is used as the gateway address. The
> > +# subsequent address up to and including end-address are
> > +# placed in the DHCP pool.
> > +#
> > +# @end-address: The DHCP IPv4 range end address to use for the
> > +#   interface. Must be in the private IP range (RFC 1918).
> > +#   Must be specified along with @start-address and
> @subnet-mask.
> > +#
> > +# @subnet-mask: The IPv4 subnet mask to use on the interface. Must
> > +#be specified along with @start-address and
> @subnet-mask.
> > +#
> > +# @isolated: Enable isolation for this interface. Interface isolation
> > +#ensures that vmnet interface is not able to communicate
> > +#with any other vmnet interfaces. Only communication with
> > +#host is allowed. Available since macOS Big Sur 11.0.
>
> Same question.
>
>
The same behavior here.



> > +#
> > +# @nat66-prefix: The IPv6 prefix to use into guest network. Must be a
> > +#unique local address i.e. start with fd00::/8 and have
> > +#length of 64.
> > +#
> > +# Since: 7.0
> > +##
> > +{ 'struct': 'NetdevVmnetSharedOptions',
> > +  'data': {
> > +'*start-address': 'str',
> > +

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

[Markus Armbruster]

Vladislav Yaroshchuk  writes:

> вт, 18 янв. 2022 г. в 18:01, Markus Armbruster :
>
>> Vladislav Yaroshchuk  writes:
>>
>> > Create separate netdevs for each vmnet operating mode:
>> > - vmnet-host
>> > - vmnet-shared
>> > - vmnet-bridged
>> >
>> > Signed-off-by: Vladislav Yaroshchuk 
>>
>> I acked v8 of the QAPI schema part.  You should add Acked-by and
>> Reviewed-by you receive in later revisions, unless you make changes that
>> invalidate them.  When in doubt, drop them.
>>
>>
> Oh ok, I'll do that next time.

Thanks :)

>> > diff --git a/qapi/net.json b/qapi/net.json
>> > index 7fab2e7cd8..b922e2e34f 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -452,6 +452,120 @@
>> >  '*vhostdev': 'str',
>> >  '*queues':   'int' } }
>> >
>> > +##
>> > +# @NetdevVmnetHostOptions:
>> > +#
>> > +# vmnet (host mode) network backend.
>> > +#
>> > +# Allows the vmnet interface to communicate with other vmnet
>> > +# interfaces that are in host mode and also with the host.
>> > +#
>> > +# @start-address: The starting IPv4 address to use for the interface.
>> > +# Must be in the private IP range (RFC 1918). Must be
>> > +# specified along with @end-address and @subnet-mask.
>> > +# This address is used as the gateway address. The
>> > +# subsequent address up to and including end-address are
>> > +# placed in the DHCP pool.
>> > +#
>> > +# @end-address: The DHCP IPv4 range end address to use for the
>> > +#   interface. Must be in the private IP range (RFC 1918).
>> > +#   Must be specified along with @start-address and
>> > +#   @subnet-mask.
>> > +#
>> > +# @subnet-mask: The IPv4 subnet mask to use on the interface. Must
>> > +#   be specified along with @start-address and @subnet-mask.
>> > +#
>> > +# @isolated: Enable isolation for this interface. Interface isolation
>> > +#ensures that vmnet interface is not able to communicate
>> > +#with any other vmnet interfaces. Only communication with
>> > +#host is allowed. Available since macOS Big Sur 11.0.
>>
>> What happens when the host is too old?
>>
>>
> In this case netdev creation will fail with
> corresponding message (error_setg() used).

"Available" feels slightly misleading.  It's always available, it just
doesn't work unless the host OS is new enough.  Suggest something like
"Requires at least macOS Big Sur 11.0."

Same for the others.

QAPI schema
Acked-by: Markus Armbruster 

[...]

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

[Markus Armbruster]

Vladislav Yaroshchuk  writes:

> Create separate netdevs for each vmnet operating mode:
> - vmnet-host
> - vmnet-shared
> - vmnet-bridged
>
> Signed-off-by: Vladislav Yaroshchuk 

I acked v8 of the QAPI schema part.  You should add Acked-by and
Reviewed-by you receive in later revisions, unless you make changes that
invalidate them.  When in doubt, drop them.

> diff --git a/qapi/net.json b/qapi/net.json
> index 7fab2e7cd8..b922e2e34f 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -452,6 +452,120 @@
>  '*vhostdev': 'str',
>  '*queues':   'int' } }
>  
> +##
> +# @NetdevVmnetHostOptions:
> +#
> +# vmnet (host mode) network backend.
> +#
> +# Allows the vmnet interface to communicate with other vmnet
> +# interfaces that are in host mode and also with the host.
> +#
> +# @start-address: The starting IPv4 address to use for the interface.
> +# Must be in the private IP range (RFC 1918). Must be
> +# specified along with @end-address and @subnet-mask.
> +# This address is used as the gateway address. The
> +# subsequent address up to and including end-address are
> +# placed in the DHCP pool.
> +#
> +# @end-address: The DHCP IPv4 range end address to use for the
> +#   interface. Must be in the private IP range (RFC 1918).
> +#   Must be specified along with @start-address and
> +#   @subnet-mask.
> +#
> +# @subnet-mask: The IPv4 subnet mask to use on the interface. Must
> +#   be specified along with @start-address and @subnet-mask.
> +#
> +# @isolated: Enable isolation for this interface. Interface isolation
> +#ensures that vmnet interface is not able to communicate
> +#with any other vmnet interfaces. Only communication with
> +#host is allowed. Available since macOS Big Sur 11.0.

What happens when the host is too old?

> +#
> +# @net-uuid: The identifier (UUID) to uniquely identify the isolated
> +#network vmnet interface should be added to. If
> +#set, no DHCP service is provided for this interface and
> +#network communication is allowed only with other interfaces
> +#added to this network identified by the UUID. Available
> +#since macOS Big Sur 11.0.

Same question.

> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'NetdevVmnetHostOptions',
> +  'data': {
> +'*start-address': 'str',
> +'*end-address':   'str',
> +'*subnet-mask':   'str',
> +'*isolated':  'bool',
> +'*net-uuid':  'str' },
> +  'if': 'CONFIG_VMNET' }
> +
> +##
> +# @NetdevVmnetSharedOptions:
> +#
> +# vmnet (shared mode) network backend.
> +#
> +# Allows traffic originating from the vmnet interface to reach the
> +# Internet through a network address translator (NAT).
> +# The vmnet interface can communicate with the host and with
> +# other shared mode interfaces on the same subnet. If no DHCP
> +# settings, subnet mask and IPv6 prefix specified, the interface can
> +# communicate with any of other interfaces in shared mode.
> +#
> +# @start-address: The starting IPv4 address to use for the interface.
> +# Must be in the private IP range (RFC 1918). Must be
> +# specified along with @end-address and @subnet-mask.
> +# This address is used as the gateway address. The
> +# subsequent address up to and including end-address are
> +# placed in the DHCP pool.
> +#
> +# @end-address: The DHCP IPv4 range end address to use for the
> +#   interface. Must be in the private IP range (RFC 1918).
> +#   Must be specified along with @start-address and @subnet-mask.
> +#
> +# @subnet-mask: The IPv4 subnet mask to use on the interface. Must
> +#be specified along with @start-address and @subnet-mask.
> +#
> +# @isolated: Enable isolation for this interface. Interface isolation
> +#ensures that vmnet interface is not able to communicate
> +#with any other vmnet interfaces. Only communication with
> +#host is allowed. Available since macOS Big Sur 11.0.

Same question.

> +#
> +# @nat66-prefix: The IPv6 prefix to use into guest network. Must be a
> +#unique local address i.e. start with fd00::/8 and have
> +#length of 64.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'NetdevVmnetSharedOptions',
> +  'data': {
> +'*start-address': 'str',
> +'*end-address':   'str',
> +'*subnet-mask':   'str',
> +'*isolated':  'bool',
> +'*nat66-prefix':  'str' },
> +  'if': 'CONFIG_VMNET' }
> +
> +##
> +# @NetdevVmnetBridgedOptions:
> +#
> +# vmnet (bridged mode) network backend.
> +#
> +# Bridges the vmnet interface with a physical network interface.
> +#
> +# @ifname: The name of the physical interface to be bridged.
> +#
> +# @isolated: Enable isolation for this interface. Interface isolation
> 

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

[Vladislav Yaroshchuk]

Hi Akihiko,

Thank you for the review! I will fix the problems and resubmit as v14.

---
Best regards,
Vladislav Yaroshchuk

пт, 14 янв. 2022 г. в 11:43, Akihiko Odaki :

> Hi,
>
> Thank you for fixing the feature availability check.
>
> I decided to just check the series thoroughly before adding Reviewed-By,
> and unfortunately ended up finding minor memory leaks and style
> problems. I'm sorry for adding comments so late.
>
> Particulalry, his patch has several 2-space indents. They should be
> 4-space. Reviews for other patch will shortly follow.
>
> Regards,
> Akihiko Odaki
>
> On 2022/01/14 2:22, Vladislav Yaroshchuk wrote:
> > Create separate netdevs for each vmnet operating mode:
> > - vmnet-host
> > - vmnet-shared
> > - vmnet-bridged
> >
> > Signed-off-by: Vladislav Yaroshchuk 
> > ---
> >   net/clients.h   |  11 
> >   net/meson.build |   7 +++
> >   net/net.c   |  10 
> >   net/vmnet-bridged.m |  25 +
> >   net/vmnet-common.m  |  20 +++
> >   net/vmnet-host.c|  24 
> >   net/vmnet-shared.c  |  25 +
> >   net/vmnet_int.h |  25 +
> >   qapi/net.json   | 133 +++-
> >   9 files changed, 278 insertions(+), 2 deletions(-)
> >   create mode 100644 net/vmnet-bridged.m
> >   create mode 100644 net/vmnet-common.m
> >   create mode 100644 net/vmnet-host.c
> >   create mode 100644 net/vmnet-shared.c
> >   create mode 100644 net/vmnet_int.h
> >
> > diff --git a/net/clients.h b/net/clients.h
> > index 92f9b59aed..c9157789f2 100644
> > --- a/net/clients.h
> > +++ b/net/clients.h
> > @@ -63,4 +63,15 @@ int net_init_vhost_user(const Netdev *netdev, const
> char *name,
> >
> >   int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> >   NetClientState *peer, Error **errp);
> > +#ifdef CONFIG_VMNET
> > +int net_init_vmnet_host(const Netdev *netdev, const char *name,
> > +  NetClientState *peer, Error **errp);
> > +
> > +int net_init_vmnet_shared(const Netdev *netdev, const char *name,
> > +  NetClientState *peer, Error **errp);
> > +
> > +int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
> > +  NetClientState *peer, Error **errp);
> > +#endif /* CONFIG_VMNET */
> > +
> >   #endif /* QEMU_NET_CLIENTS_H */
> > diff --git a/net/meson.build b/net/meson.build
> > index 847bc2ac85..00a88c4951 100644
> > --- a/net/meson.build
> > +++ b/net/meson.build
> > @@ -42,4 +42,11 @@ softmmu_ss.add(when: 'CONFIG_POSIX', if_true:
> files(tap_posix))
> >   softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('tap-win32.c'))
> >   softmmu_ss.add(when: 'CONFIG_VHOST_NET_VDPA', if_true:
> files('vhost-vdpa.c'))
> >
> > +vmnet_files = files(
> > +  'vmnet-common.m',
> > +  'vmnet-bridged.m',
> > +  'vmnet-host.c',
> > +  'vmnet-shared.c'
> > +)
> > +softmmu_ss.add(when: vmnet, if_true: vmnet_files)
> >   subdir('can')
> > diff --git a/net/net.c b/net/net.c
> > index f0d14dbfc1..1dbb64b935 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1021,6 +1021,11 @@ static int (* const
> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
> >   #ifdef CONFIG_L2TPV3
> >   [NET_CLIENT_DRIVER_L2TPV3]= net_init_l2tpv3,
> >   #endif
> > +#ifdef CONFIG_VMNET
> > +[NET_CLIENT_DRIVER_VMNET_HOST] = net_init_vmnet_host,
> > +[NET_CLIENT_DRIVER_VMNET_SHARED] = net_init_vmnet_shared,
> > +[NET_CLIENT_DRIVER_VMNET_BRIDGED] = net_init_vmnet_bridged,
> > +#endif /* CONFIG_VMNET */
> >   };
> >
> >
> > @@ -1106,6 +,11 @@ void show_netdevs(void)
> >   #endif
> >   #ifdef CONFIG_VHOST_VDPA
> >   "vhost-vdpa",
> > +#endif
> > +#ifdef CONFIG_VMNET
> > +"vmnet-host",
> > +"vmnet-shared",
> > +"vmnet-bridged",
> >   #endif
> >   };
> >
> > diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
> > new file mode 100644
> > index 00..4e42a90391
> > --- /dev/null
> > +++ b/net/vmnet-bridged.m
> > @@ -0,0 +1,25 @@
> > +/*
> > + * vmnet-bridged.m
> > + *
> > + * Copyright(c) 2021 Vladislav Yaroshchuk 
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/qapi-types-net.h"
> > +#include "vmnet_int.h"
> > +#include "clients.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/error.h"
> > +
> > +#include 
> > +
> > +int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
> > +   NetClientState *peer, Error **errp)
> > +{
> > +  error_setg(errp, "vmnet-bridged is not implemented yet");
> > +  return -1;
> > +}
> > diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> > new file mode 100644
> > index 00..532d152840
> > --- /dev/null
> > +++ b/net/vmnet-common.m
> > @@ -0,0 +1,20 @@
> > +/*
> > + * vmnet-common.m - network client wrapper for Apple 

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

[Akihiko Odaki]

Hi,

Thank you for fixing the feature availability check.

I decided to just check the series thoroughly before adding Reviewed-By, 
and unfortunately ended up finding minor memory leaks and style 
problems. I'm sorry for adding comments so late.


Particulalry, his patch has several 2-space indents. They should be 
4-space. Reviews for other patch will shortly follow.


Regards,
Akihiko Odaki

On 2022/01/14 2:22, Vladislav Yaroshchuk wrote:

Create separate netdevs for each vmnet operating mode:
- vmnet-host
- vmnet-shared
- vmnet-bridged

Signed-off-by: Vladislav Yaroshchuk 
---
  net/clients.h   |  11 
  net/meson.build |   7 +++
  net/net.c   |  10 
  net/vmnet-bridged.m |  25 +
  net/vmnet-common.m  |  20 +++
  net/vmnet-host.c|  24 
  net/vmnet-shared.c  |  25 +
  net/vmnet_int.h |  25 +
  qapi/net.json   | 133 +++-
  9 files changed, 278 insertions(+), 2 deletions(-)
  create mode 100644 net/vmnet-bridged.m
  create mode 100644 net/vmnet-common.m
  create mode 100644 net/vmnet-host.c
  create mode 100644 net/vmnet-shared.c
  create mode 100644 net/vmnet_int.h

diff --git a/net/clients.h b/net/clients.h
index 92f9b59aed..c9157789f2 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -63,4 +63,15 @@ int net_init_vhost_user(const Netdev *netdev, const char 
*name,
  
  int net_init_vhost_vdpa(const Netdev *netdev, const char *name,

  NetClientState *peer, Error **errp);
+#ifdef CONFIG_VMNET
+int net_init_vmnet_host(const Netdev *netdev, const char *name,
+  NetClientState *peer, Error **errp);
+
+int net_init_vmnet_shared(const Netdev *netdev, const char *name,
+  NetClientState *peer, Error **errp);
+
+int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
+  NetClientState *peer, Error **errp);
+#endif /* CONFIG_VMNET */
+
  #endif /* QEMU_NET_CLIENTS_H */
diff --git a/net/meson.build b/net/meson.build
index 847bc2ac85..00a88c4951 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -42,4 +42,11 @@ softmmu_ss.add(when: 'CONFIG_POSIX', if_true: 
files(tap_posix))
  softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('tap-win32.c'))
  softmmu_ss.add(when: 'CONFIG_VHOST_NET_VDPA', if_true: files('vhost-vdpa.c'))
  
+vmnet_files = files(

+  'vmnet-common.m',
+  'vmnet-bridged.m',
+  'vmnet-host.c',
+  'vmnet-shared.c'
+)
+softmmu_ss.add(when: vmnet, if_true: vmnet_files)
  subdir('can')
diff --git a/net/net.c b/net/net.c
index f0d14dbfc1..1dbb64b935 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1021,6 +1021,11 @@ static int (* const 
net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
  #ifdef CONFIG_L2TPV3
  [NET_CLIENT_DRIVER_L2TPV3]= net_init_l2tpv3,
  #endif
+#ifdef CONFIG_VMNET
+[NET_CLIENT_DRIVER_VMNET_HOST] = net_init_vmnet_host,
+[NET_CLIENT_DRIVER_VMNET_SHARED] = net_init_vmnet_shared,
+[NET_CLIENT_DRIVER_VMNET_BRIDGED] = net_init_vmnet_bridged,
+#endif /* CONFIG_VMNET */
  };
  
  
@@ -1106,6 +,11 @@ void show_netdevs(void)

  #endif
  #ifdef CONFIG_VHOST_VDPA
  "vhost-vdpa",
+#endif
+#ifdef CONFIG_VMNET
+"vmnet-host",
+"vmnet-shared",
+"vmnet-bridged",
  #endif
  };
  
diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m

new file mode 100644
index 00..4e42a90391
--- /dev/null
+++ b/net/vmnet-bridged.m
@@ -0,0 +1,25 @@
+/*
+ * vmnet-bridged.m
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include 
+
+int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
+   NetClientState *peer, Error **errp)
+{
+  error_setg(errp, "vmnet-bridged is not implemented yet");
+  return -1;
+}
diff --git a/net/vmnet-common.m b/net/vmnet-common.m
new file mode 100644
index 00..532d152840
--- /dev/null
+++ b/net/vmnet-common.m
@@ -0,0 +1,20 @@
+/*
+ * vmnet-common.m - network client wrapper for Apple vmnet.framework
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk 
+ * Copyright(c) 2021 Phillip Tennen 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include 
+
diff --git a/net/vmnet-host.c b/net/vmnet-host.c
new file mode 100644
index 00..4a5ef99dc7
--- /dev/null
+++ b/net/vmnet-host.c
@@ -0,0 +1,24 @@
+/*
+ * vmnet-host.c
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk 
+ *
+ * This work is licensed under 

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

[Vladislav Yaroshchuk]

Create separate netdevs for each vmnet operating mode:
- vmnet-host
- vmnet-shared
- vmnet-bridged

Signed-off-by: Vladislav Yaroshchuk 
---
 net/clients.h   |  11 
 net/meson.build |   7 +++
 net/net.c   |  10 
 net/vmnet-bridged.m |  25 +
 net/vmnet-common.m  |  20 +++
 net/vmnet-host.c|  24 
 net/vmnet-shared.c  |  25 +
 net/vmnet_int.h |  25 +
 qapi/net.json   | 133 +++-
 9 files changed, 278 insertions(+), 2 deletions(-)
 create mode 100644 net/vmnet-bridged.m
 create mode 100644 net/vmnet-common.m
 create mode 100644 net/vmnet-host.c
 create mode 100644 net/vmnet-shared.c
 create mode 100644 net/vmnet_int.h

diff --git a/net/clients.h b/net/clients.h
index 92f9b59aed..c9157789f2 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -63,4 +63,15 @@ int net_init_vhost_user(const Netdev *netdev, const char 
*name,
 
 int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
 NetClientState *peer, Error **errp);
+#ifdef CONFIG_VMNET
+int net_init_vmnet_host(const Netdev *netdev, const char *name,
+  NetClientState *peer, Error **errp);
+
+int net_init_vmnet_shared(const Netdev *netdev, const char *name,
+  NetClientState *peer, Error **errp);
+
+int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
+  NetClientState *peer, Error **errp);
+#endif /* CONFIG_VMNET */
+
 #endif /* QEMU_NET_CLIENTS_H */
diff --git a/net/meson.build b/net/meson.build
index 847bc2ac85..00a88c4951 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -42,4 +42,11 @@ softmmu_ss.add(when: 'CONFIG_POSIX', if_true: 
files(tap_posix))
 softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('tap-win32.c'))
 softmmu_ss.add(when: 'CONFIG_VHOST_NET_VDPA', if_true: files('vhost-vdpa.c'))
 
+vmnet_files = files(
+  'vmnet-common.m',
+  'vmnet-bridged.m',
+  'vmnet-host.c',
+  'vmnet-shared.c'
+)
+softmmu_ss.add(when: vmnet, if_true: vmnet_files)
 subdir('can')
diff --git a/net/net.c b/net/net.c
index f0d14dbfc1..1dbb64b935 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1021,6 +1021,11 @@ static int (* const 
net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
 #ifdef CONFIG_L2TPV3
 [NET_CLIENT_DRIVER_L2TPV3]= net_init_l2tpv3,
 #endif
+#ifdef CONFIG_VMNET
+[NET_CLIENT_DRIVER_VMNET_HOST] = net_init_vmnet_host,
+[NET_CLIENT_DRIVER_VMNET_SHARED] = net_init_vmnet_shared,
+[NET_CLIENT_DRIVER_VMNET_BRIDGED] = net_init_vmnet_bridged,
+#endif /* CONFIG_VMNET */
 };
 
 
@@ -1106,6 +,11 @@ void show_netdevs(void)
 #endif
 #ifdef CONFIG_VHOST_VDPA
 "vhost-vdpa",
+#endif
+#ifdef CONFIG_VMNET
+"vmnet-host",
+"vmnet-shared",
+"vmnet-bridged",
 #endif
 };
 
diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
new file mode 100644
index 00..4e42a90391
--- /dev/null
+++ b/net/vmnet-bridged.m
@@ -0,0 +1,25 @@
+/*
+ * vmnet-bridged.m
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include 
+
+int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
+   NetClientState *peer, Error **errp)
+{
+  error_setg(errp, "vmnet-bridged is not implemented yet");
+  return -1;
+}
diff --git a/net/vmnet-common.m b/net/vmnet-common.m
new file mode 100644
index 00..532d152840
--- /dev/null
+++ b/net/vmnet-common.m
@@ -0,0 +1,20 @@
+/*
+ * vmnet-common.m - network client wrapper for Apple vmnet.framework
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk 
+ * Copyright(c) 2021 Phillip Tennen 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include 
+
diff --git a/net/vmnet-host.c b/net/vmnet-host.c
new file mode 100644
index 00..4a5ef99dc7
--- /dev/null
+++ b/net/vmnet-host.c
@@ -0,0 +1,24 @@
+/*
+ * vmnet-host.c
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include 
+
+int net_init_vmnet_host(const Netdev *netdev, const char *name,
+NetClientState *peer, Error **errp) {
+  error_setg(errp, "vmnet-host is not implemented yet");
+  return -1;