GNU bug report logs - #52470
[PATCH] services: bluetooth: Add missing config parameters

Previous Next

Full log

View this message in rfc822 format

From: Demis Balbach <db <at> minikn.xyz>
To: 52470 <at> debbugs.gnu.org
Subject: [bug#52470] [PATCH] services: bluetooth: Add missing config parameters
Date: Sun, 19 Dec 2021 14:11:49 +0100

[Message part 1 (text/plain, inline)]

Hello Liliana,

thanks for your input on https://issues.guix.gnu.org/52575.
I will continue the discussion about it here.

You prepend most of your comments with "Why not do ...". I have an
universal answer for that: I don't know :) I did what I did with the
limited Guile knowledge at my disposal. I'm still learning everyday and
therefore am thankful for your recommendations.

> Is it really a good idea to use a string-encoded number here?  Why not
> a number?  Why not two?  A pair or list of symbols mayhaps?
It should be a number, you are correct.

> You maybe want to have a <device-id> record here, but fair enough, a
> string works too.
I can see the benefit of that, but maybe that's something for another
patch. I can add a `;;; MAYBE: ` comment to the implementation however
if that's desired.

> Why have a key network/on?  Why not use 'on and 'network and document,
> that they function the same due to (insert implementation detail).
I think this point we need to discuss. I tried to implement things as
close as possible to the "original" keys/values in
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/main.conf

Specifically for the `privacy` key, you can see the implementation here
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/main.conf#n68,

in particular

--8<---------------cut here---------------start------------->8---
# Possible values for LE mode: "off", "network/on", "device"
# Possible values for Dual mode: "off", "network/on", "device",
# "limited-network", "limited-device"
--8<---------------cut here---------------end--------------->8---

Now, I don't think it's a good idea to introduce arbitrary values for
Guix, meaning split `network/on` in `'network` and `'on`. In my opinion
(which may very well be wrong) the config values applicable in Guix
should match those in the resulting config file as close as possible.

For a user coming to Guix who already worked with the bluetooth configuration on
another distro, seeing `'network` and/or `'on` (but not `'network/on`)
as possible config values, is confusing, at least I think it is.

> Why a list of UUIDs?  Wouldn't it make more sense to use symbols like
> 'debug, 'll-privacy, 'quality-report, etc.?
> On that note, we have a UUID type, so use it.
I actually didn't know there's a UUID type. This makes more sense. But
not a list as only one value can be applied. Also I would advise
introducing symbol like `'ll-privacy` and the like because of what I
said earlier.

> Should be a boolean.
Same point here. I used boolean types whereever the config value need
was `true`/`false`. In this particular case (I don't know why) the
bluetooth implementations expects `0`/`1` instead of the former. This is
why I kept it a number. However, in this case I think it's okay to
deviate from the original implementation because `0` and false / `1` and
`true` are universally understood as being equivalent (By this of course
I mean the user would set `#f`/`#t`, the written value however would
still be `0`/`1`).

> Might want to rename/alias 'yes to 'paired.
Again, same point here. `'paired` would be an arbitrary value only Guix
have.

> You probably want to write that as "an integer 7 <= N <= 16".
...
>As above.
Yes, absolutely.

> Again, make sure that your code works with UUIDs here if it doesn't
> already.
Will do.

> Use numbers, not strings.
Will do.

I'll create a new patch with all the things altered I agree about. I'll
leave the rest until we have talked about the points I made. 

-- 
Best regards / Mit freundlichen Grüßen,
Demis Balbach

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.