diff mbox series

coreaudio: Always return 0 in handle_voice_change

Message ID 20220306063949.28138-1-akihiko.odaki@gmail.com (mailing list archive)
State New, archived
Headers show
Series coreaudio: Always return 0 in handle_voice_change | expand

Commit Message

Akihiko Odaki March 6, 2022, 6:39 a.m. UTC 
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(-)

Comments

Christian Schoenebeck March 6, 2022, 10:49 a.m. UTC | #1 
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,
Akihiko Odaki March 6, 2022, 10:54 a.m. UTC | #2 
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,
> 
>
Christian Schoenebeck March 6, 2022, 12:16 p.m. UTC | #3 
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
Akihiko Odaki March 6, 2022, 12:30 p.m. UTC | #4 
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
>
>
diff mbox series

Patch

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,