On Sat Feb 15 21:55:59 2025 +0000, Elizabeth Figura wrote:
Sorry for the late review, I don't have any good excuse. In general, these ioctls are a bit odd without the code on the bluetoothapis side to hook them up. It's probably reasonable to add those to this merge request. From 1/4:
+NTSTATUS winebluetooth_radio_set_property( winebluetooth_radio_t radio, + winebluetooth_radio_props_mask_t prop, + union winebluetooth_property *property )The "property" parameter is not used.
+ switch (prop) + { + case WINEBLUETOOTH_RADIO_PROPERTY_CONNECTABLE: + case WINEBLUETOOTH_RADIO_PROPERTY_DISCOVERABLE: + case WINEBLUETOOTH_RADIO_PROPERTY_PAIRABLE: + break; + default: + return STATUS_INVALID_PARAMETER; + }The Unix side returns STATUS_INVALID_PARAMETER for the others anyway, so you might as well just get rid of this whole switch.
+ case LOCAL_RADIO_CONNECTABLE: + status = + winebluetooth_radio_set_property( ext->radio, WINEBLUETOOTH_RADIO_PROPERTY_CONNECTABLE, &prop_value ); + + break; + case LOCAL_RADIO_DISCOVERABLE: + status = + winebluetooth_radio_set_property( ext->radio, WINEBLUETOOTH_RADIO_PROPERTY_DISCOVERABLE, &prop_value ); + break;Why are we translating to a different flags enum? Why not pass this flag enum directly? You also implement the PAIRABLE property, but that code is never reached.
+ status = UNIX_BLUETOOTH_CALL( bluetooth_adapter_set_prop, ¶ms ); + + if (status != STATUS_SUCCESS) return status; + + return STATUS_SUCCESS;This could just be "return UNIX_BLUETOOTH_CALL( ... )". To each their own style, I guess, but in general there are a lot of helpers that seem quite redundant... From 2/4:
+ if (filter->le && filter->bredr) + transport = WINEBLUETOOTH_DISCOVERY_TRANSPORT_AUTO; + else if (filter->le) + transport = WINEBLUETOOTH_DISCOVERY_TRANSPORT_LE; + else if (filter->bredr) + transport = WINEBLUETOOTH_DISCOVERY_TRANSPORT_BR_EDR; + else + transport = WINEBLUETOOTH_DISCOVERY_TRANSPORT_DEFAULT;Why convert to an intermediate set of flags instead of just passing the params directly to the Unix side? Aside from the fact that the bitfield might not work, but it's pretty unnecessary anyway.
No worries!
In general, these ioctls are a bit odd without the code on the bluetoothapis side to hook them up. It's probably reasonable to add those to this merge request.
Makes sense. I have shortened this MR to only include `IOCTL_WINEBTH_RADIO_SET_FLAG`, as userspace support for the discovery ones require quite a bit of more code.
Why are we translating to a different flags enum? Why not pass this flag enum directly?
To keep it consistent. The `WINEBLUETOOTH_RADIO_PROPERTY` constants get used to inform the driver about property changes (`bluez_filter`, `bluetooth_radio_set_properties`), and to check if the properties for the adapter are known in `IOCTL_BTH_GET_LOCAL_INFO`. So, I thought it'd be better if the unix/dbus code only used the `WINEBLUETOOTH_*` constants.
Why convert to an intermediate set of flags instead of just passing the params directly to the Unix side?
Yeah, I'm also not sure if the bitfields would work across unixlib boundaries. I can just use `BOOL`s for LE and BR/EDR instead if that's better.
The "property" parameter is not used. The Unix side returns STATUS_INVALID_PARAMETER for the others anyway, so you might as well just get rid of this whole switch.
Fixed, thanks!