[PATCH] coreaudio: Always return 0 in handle_voice_change

[PATCH] coreaudio: Always return 0 in handle_voice_change

[Akihiko Odaki]

MacOSX.sdk/System/Library/Frameworks/CoreAudio.framework/Headers/AudioHardware.h
says:
> The return value is currently unused and should always be 0.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 audio/coreaudio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/audio/coreaudio.c b/audio/coreaudio.c
index 0f19d0ce01c..91445096358 100644
--- a/audio/coreaudio.c
+++ b/audio/coreaudio.c
@@ -540,7 +540,6 @@ static OSStatus handle_voice_change(
     const AudioObjectPropertyAddress *in_addresses,
     void *in_client_data)
 {
-    OSStatus status;
     coreaudioVoiceOut *core = in_client_data;
 
     qemu_mutex_lock_iothread();
@@ -549,13 +548,12 @@ static OSStatus handle_voice_change(
         fini_out_device(core);
     }
 
-    status = init_out_device(core);
-    if (!status) {
+    if (!init_out_device(core)) {
         update_device_playback_state(core);
     }
 
     qemu_mutex_unlock_iothread();
-    return status;
+    return 0;
 }
 
 static int coreaudio_init_out(HWVoiceOut *hw, struct audsettings *as,
-- 
2.32.0 (Apple Git-132)

Re: [PATCH] coreaudio: Always return 0 in handle_voice_change

[Christian Schoenebeck]

On Sonntag, 6. März 2022 07:39:49 CET Akihiko Odaki wrote:
> MacOSX.sdk/System/Library/Frameworks/CoreAudio.framework/Headers/AudioHardwa
> re.h
> says:
> > The return value is currently unused and should always be 0.

Where does it say that, about which macOS functions? There are quite a bunch 
of macOS functions involved in init_out_device(), and they all have error 
pathes in init_out_device(), and they still will have them after this patch.

And again, I'm missing: this as an improvement because? Is this a user 
invisible improvement (e.g. removing redundant branches), or is this a user 
visible improvement, i.e. does it fix a misbehaviour? In case of the latter, 
which misbehaviour did you encounter?

Best regards,
Christian Schoenebeck

> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> ---
>  audio/coreaudio.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/audio/coreaudio.c b/audio/coreaudio.c
> index 0f19d0ce01c..91445096358 100644
> --- a/audio/coreaudio.c
> +++ b/audio/coreaudio.c
> @@ -540,7 +540,6 @@ static OSStatus handle_voice_change(
>      const AudioObjectPropertyAddress *in_addresses,
>      void *in_client_data)
>  {
> -    OSStatus status;
>      coreaudioVoiceOut *core = in_client_data;
> 
>      qemu_mutex_lock_iothread();
> @@ -549,13 +548,12 @@ static OSStatus handle_voice_change(
>          fini_out_device(core);
>      }
> 
> -    status = init_out_device(core);
> -    if (!status) {
> +    if (!init_out_device(core)) {
>          update_device_playback_state(core);
>      }
> 
>      qemu_mutex_unlock_iothread();
> -    return status;
> +    return 0;
>  }
> 
>  static int coreaudio_init_out(HWVoiceOut *hw, struct audsettings *as,

Re: [PATCH] coreaudio: Always return 0 in handle_voice_change

[Akihiko Odaki]

On 2022/03/06 19:49, Christian Schoenebeck wrote:
> On Sonntag, 6. März 2022 07:39:49 CET Akihiko Odaki wrote:
>> MacOSX.sdk/System/Library/Frameworks/CoreAudio.framework/Headers/AudioHardwa
>> re.h
>> says:
>>> The return value is currently unused and should always be 0.
> 
> Where does it say that, about which macOS functions? There are quite a bunch
> of macOS functions involved in init_out_device(), and they all have error
> pathes in init_out_device(), and they still will have them after this patch.
> 
> And again, I'm missing: this as an improvement because? Is this a user
> invisible improvement (e.g. removing redundant branches), or is this a user
> visible improvement, i.e. does it fix a misbehaviour? In case of the latter,
> which misbehaviour did you encounter?

handle_voice_change itself is a callback.
It is invisible for a user since "the return value is currently unused".

Regards,
Akihiko Odaki

> 
> Best regards,
> Christian Schoenebeck
> 
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
>> ---
>>   audio/coreaudio.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/audio/coreaudio.c b/audio/coreaudio.c
>> index 0f19d0ce01c..91445096358 100644
>> --- a/audio/coreaudio.c
>> +++ b/audio/coreaudio.c
>> @@ -540,7 +540,6 @@ static OSStatus handle_voice_change(
>>       const AudioObjectPropertyAddress *in_addresses,
>>       void *in_client_data)
>>   {
>> -    OSStatus status;
>>       coreaudioVoiceOut *core = in_client_data;
>>
>>       qemu_mutex_lock_iothread();
>> @@ -549,13 +548,12 @@ static OSStatus handle_voice_change(
>>           fini_out_device(core);
>>       }
>>
>> -    status = init_out_device(core);
>> -    if (!status) {
>> +    if (!init_out_device(core)) {
>>           update_device_playback_state(core);
>>       }
>>
>>       qemu_mutex_unlock_iothread();
>> -    return status;
>> +    return 0;
>>   }
>>
>>   static int coreaudio_init_out(HWVoiceOut *hw, struct audsettings *as,
> 
> 

Re: [PATCH] coreaudio: Always return 0 in handle_voice_change

[Christian Schoenebeck]

On Sonntag, 6. März 2022 11:54:00 CET Akihiko Odaki wrote:
> On 2022/03/06 19:49, Christian Schoenebeck wrote:
> > On Sonntag, 6. März 2022 07:39:49 CET Akihiko Odaki wrote:
> >> MacOSX.sdk/System/Library/Frameworks/CoreAudio.framework/Headers/AudioHar
> >> dwa re.h
> >> 
> >> says:
> >>> The return value is currently unused and should always be 0.
> > 
> > Where does it say that, about which macOS functions? There are quite a
> > bunch of macOS functions involved in init_out_device(), and they all have
> > error pathes in init_out_device(), and they still will have them after
> > this patch.
> > 
> > And again, I'm missing: this as an improvement because? Is this a user
> > invisible improvement (e.g. removing redundant branches), or is this a
> > user
> > visible improvement, i.e. does it fix a misbehaviour? In case of the
> > latter, which misbehaviour did you encounter?
> 
> handle_voice_change itself is a callback.
> It is invisible for a user since "the return value is currently unused".

Then the commit log should be more specific and say something like:

"
handle_voice_change() is a CoreAudio callback function as of CoreAudio type 
'AudioObjectPropertyListenerProc', and for the latter MacOSX.sdk/System/
Library/Frameworks/CoreAudio.framework/Headers/AudioHardware.h
says 'The return value is currently unused and should always be 0.'.
"

Nevertheless, personally I would not change that, but I won't object either.

I read it like "The CoreAudio subsystem of macOS currently ignores the result 
of your callback, and for that reason simply return 0 for now.". It does not 
say "you must not return anything else than 0". ATM it simply does not matter 
what you return here.

Best regards,
Christian Schoenebeck

Re: [PATCH] coreaudio: Always return 0 in handle_voice_change

[Akihiko Odaki]

On Sun, Mar 6, 2022 at 9:16 PM Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> On Sonntag, 6. März 2022 11:54:00 CET Akihiko Odaki wrote:
> > On 2022/03/06 19:49, Christian Schoenebeck wrote:
> > > On Sonntag, 6. März 2022 07:39:49 CET Akihiko Odaki wrote:
> > >> MacOSX.sdk/System/Library/Frameworks/CoreAudio.framework/Headers/AudioHar
> > >> dwa re.h
> > >>
> > >> says:
> > >>> The return value is currently unused and should always be 0.
> > >
> > > Where does it say that, about which macOS functions? There are quite a
> > > bunch of macOS functions involved in init_out_device(), and they all have
> > > error pathes in init_out_device(), and they still will have them after
> > > this patch.
> > >
> > > And again, I'm missing: this as an improvement because? Is this a user
> > > invisible improvement (e.g. removing redundant branches), or is this a
> > > user
> > > visible improvement, i.e. does it fix a misbehaviour? In case of the
> > > latter, which misbehaviour did you encounter?
> >
> > handle_voice_change itself is a callback.
> > It is invisible for a user since "the return value is currently unused".
>
> Then the commit log should be more specific and say something like:
>
> "
> handle_voice_change() is a CoreAudio callback function as of CoreAudio type
> 'AudioObjectPropertyListenerProc', and for the latter MacOSX.sdk/System/
> Library/Frameworks/CoreAudio.framework/Headers/AudioHardware.h
> says 'The return value is currently unused and should always be 0.'.

That makes sense. I'll submit v2 soon.

> "
>
> Nevertheless, personally I would not change that, but I won't object either.
>
> I read it like "The CoreAudio subsystem of macOS currently ignores the result
> of your callback, and for that reason simply return 0 for now.". It does not
> say "you must not return anything else than 0". ATM it simply does not matter
> what you return here.

I think it is dangerous to guess something not described in the
comment. It can lead to possible breakage if the guess turns out false
and a future Core Audio version decides to do something unintended
when it is not 0. OStatus is just an integer type and does not have a
particular semantics like enum types and errno so it is impossible to
know possible consequences in the case. Returning 0 as documented is
possibly safer and not harmful at least.

Regards,
Akihiko Odaki

>
> Best regards,
> Christian Schoenebeck
>
>