Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via email, send a message with subject or
body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH 2/3] vpn-provider: Implement setting of multiple VPN
properties with one call
(Jussi Laakkonen)
2. Re: [PATCH 2/3] vpn-provider: Implement setting of multiple VPN
properties with one call
(Daniel Wagner)
3. Re: [PATCH 2/3 v2] vpn-provider: Implement setting of multiple VPN
properties with one call
(Jussi Laakkonen)
4. Re: [PATCH 2/3] vpn-provider: Implement setting of multiple VPN
properties with one call
(Jussi Laakkonen)
5. Re: [PATCH 3/3 v2] doc: Describe property array use with VPN connection
SetProperty D-Bus method
(Jussi Laakkonen)
6. Re: [PATCH 3/3 v2] doc: Describe property array use with VPN connection
SetProperty D-Bus method
(Daniel Wagner)
7. Re: [PATCH 2/3 v2] vpn-provider: Implement setting of multiple VPN
properties with one call
(Daniel Wagner)
----------------------------------------------------------------------
Date: Mon, 30 Dec 2019 13:24:53 +0200
From: Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH 2/3] vpn-provider: Implement setting of multiple
VPN properties with one call
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel
On 12/21/19 1:25 PM, Daniel Wagner wrote:
> I think we are on the same page regarding how the method call should
> behave. So it's either everything okay or something went wrong. If
> something went wrong nothing will change, correct?
>
> And with that in mind I looked at the code and if I am not mistaken the
> 'nothing will change' promise is not completely fulfilled. The change
> wont be written on disk but some changes might have been done on the
> config settings in memory.
I guess I documented the behavior more accurately in the v2 of this
patch as that was accepted. My point was that reverting the changes in
case there is even one error (with the property values) might not be the
best way to do it. Instead, writing the changes to disk when at least
one of the values has been changed, and doing the write only once,
seemed better. One of the reasons was that user changing the values
could be prompted to correct the values, if the UI checks are lacking -
or to better inform the developer that better checks for some property
values in the UI are needed.
Handling these error cases is quite challenging. It would be good to
have the property changes done as an atomic transaction, but it would
mean checking the values first, and writing a property validator for
this. For me this did not make sense as all of the properties are not
required to be present in the dict, so the property set to be changed
does not have to be complete. So there can be a small set of property
values set, which indicates that partial changes VPN property values are
allowed and thus, some values can be ignored and for those property
values that were changed the PropertyChanged signal is emitted. And if
UI does have detection that which property values were not changed, then
these can be informed to user and a reduced set with corrected values
can be sent.
After all, the main motivation was to avoid the unnecessary disk writes
when a large set of properties are to be changed at once. Depending on
the UI design, the change can be immediate after changing one value, or
the changes are sent as a whole, without comparing to the previous value
of a property at the UI level. My aim was to have this as flexible as it
seems reasonable.
However, I understand the desire to have atomic transactions here as
well in order to ensure that all requested property value changes are
done, or nothing is done. This could be a part of TODOs as well, to
extend both service.c and vpn-provider.c property value setting D-Bus
methods to have similar functionality. What do you think Daniel, should
the atomic transaction approach be applied to both?
>
>> P.S. The first reply on 1/3 arrived to my mailbox, this 2/3 reply
>> didn't :)
>
> Argh, I've complained once more about it. If nothing happens we might
> need to move to another hosting service.
Something has been apparently done as now the replies, and all other
messages sent to the list as well, are arriving to my mailbox. I think
before holidays I did not even see the v2 of the 3/3 patch in the web
app at all.
Cheers,
Jussi
------------------------------
Date: Mon, 30 Dec 2019 13:32:21 +0100
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH 2/3] vpn-provider: Implement setting of multiple
VPN properties with one call
To: Jussi Laakkonen <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Jussi,
On 30.12.19 12:24, Jussi Laakkonen wrote:
> On 12/21/19 1:25 PM, Daniel Wagner wrote:
>> I think we are on the same page regarding how the method call should
>> behave. So it's either everything okay or something went wrong. If
>> something went wrong nothing will change, correct?
>>
>> And with that in mind I looked at the code and if I am not mistaken
>> the 'nothing will change' promise is not completely fulfilled. The
>> change wont be written on disk but some changes might have been done
>> on the config settings in memory.
>
> I guess I documented the behavior more accurately in the v2 of this
> patch as that was accepted. My point was that reverting the changes in
> case there is even one error (with the property values) might not be the
> best way to do it. Instead, writing the changes to disk when at least
> one of the values has been changed, and doing the write only once,
> seemed better. One of the reasons was that user changing the values
> could be prompted to correct the values, if the UI checks are lacking -
> or to better inform the developer that better checks for some property
> values in the UI are needed.
Yes, this makes perfectly sense.
> Handling these error cases is quite challenging. It would be good to
> have the property changes done as an atomic transaction, but it would
> mean checking the values first, and writing a property validator for
> this. For me this did not make sense as all of the properties are not
> required to be present in the dict, so the property set to be changed
> does not have to be complete. So there can be a small set of property
> values set, which indicates that partial changes VPN property values are
> allowed and thus, some values can be ignored and for those property
> values that were changed the PropertyChanged signal is emitted. And if
> UI does have detection that which property values were not changed, then
> these can be informed to user and a reduced set with corrected values
> can be sent.
Indeed, the error handling is very difficult. I was only looking at the
patch itself without considering the big picture. So the thing I was
pondering on was following:
[ a: "foo", b: "bar", UserRoutes: [...] ]
'a' and 'b' will be parsed and stored via vpn_provider_set_string(). If
there is an error in UserRoutes parsing, 'a' and 'b' will not set the
the old value (reset).
As I understand the error will be reported via an D-Bus error message
and there is no disk write at this point for 'a' and 'b'.
> After all, the main motivation was to avoid the unnecessary disk writes
> when a large set of properties are to be changed at once. Depending on
> the UI design, the change can be immediate after changing one value, or
> the changes are sent as a whole, without comparing to the previous value
> of a property at the UI level. My aim was to have this as flexible as it
> seems reasonable.
Again, I am all fine with this.
> However, I understand the desire to have atomic transactions here as
> well in order to ensure that all requested property value changes are
> done, or nothing is done. This could be a part of TODOs as well, to
> extend both service.c and vpn-provider.c property value setting D-Bus
> methods to have similar functionality. What do you think Daniel, should
> the atomic transaction approach be applied to both?
Not sure if we enter the over engineering area here :) You fixed your
initial problem (reducing the disk writes) and the error handling
is still as good it was. Let's see how this works and if we see real
world problems we can still address them.
>>> P.S. The first reply on 1/3 arrived to my mailbox, this 2/3 reply
>>> didn't :)
>>
>> Argh, I've complained once more about it. If nothing happens we might
>> need to move to another hosting service.
>
> Something has been apparently done as now the replies, and all other
> messages sent to the list as well, are arriving to my mailbox. I think
> before holidays I did not even see the v2 of the 3/3 patch in the web
> app at all.
Ah, good to know. I haven't good any feedback but maybe some admin was
able to look into the setup.
Thanks,
Daniel
------------------------------
Date: Mon, 30 Dec 2019 14:56:08 +0200
From: Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH 2/3 v2] vpn-provider: Implement setting of
multiple VPN properties with one call
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel,
On 12/21/19 1:53 PM, Daniel Wagner wrote:
> The missing concept in the Property API for changing multiple settings
> at once leads to a bit strange things. Obvioulsy, it would be more
> naturual to have single value set/get/clear/changed and the same thing
> for multiple properties at once. Well, since this doesn't exist
> neither for ConnMan nor for the freedesktop API let's go with this.
>
> Maybe we should bring this up at freedesktop and see what the
> maintainers of the Property API think about it.
>
Having the methods for multiple properties would offer more flexibility,
for the use of the method and in the underlying implementation. So yes,
I'd agree that could be proposed in freedesktop as well.
Specification [1] does, however, state, that there is a
PropertiesChanged signal already in the D-Bus Properties interface
(since version 0.14 in 2010) that can be used to indicate which
properties were changed and which properties were invalidated (array of
strings). So, the error reporting for the need of setting multiple
properties at once (and clearing etc.) is already speficied. This may be
good to implement in connman as well, when service.c would support
setting multiple properties in one call.
From the looks of it, PropertiesChanged is used only by the network
manager compatiblity plugin (plugins/nmcompat.c) and gsupplicant.
PropertiesChanged would not fit to all situations but to the ones where
multiple properties are changed in any way.
Cheers,
Jussi
[1]
https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties
------------------------------
Date: Mon, 30 Dec 2019 14:58:25 +0200
From: Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH 2/3] vpn-provider: Implement setting of multiple
VPN properties with one call
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel,
On 12/30/19 2:32 PM, Daniel Wagner wrote:
> Indeed, the error handling is very difficult. I was only looking at the
> patch itself without considering the big picture. So the thing I was
> pondering on was following:
>
> [ a: "foo", b: "bar", UserRoutes: [...] ]
>
> 'a' and 'b' will be parsed and stored via vpn_provider_set_string(). If
> there is an error in UserRoutes parsing, 'a' and 'b' will not set the
> the old value (reset).
>
> As I understand the error will be reported via an D-Bus error message
> and there is no disk write at this point for 'a' and 'b'.
>
Correct :)
>
> Not sure if we enter the over engineering area here :) You fixed your
> initial problem (reducing the disk writes) and the error handling
> is still as good it was. Let's see how this works and if we see real
> world problems we can still address them.
>
Mondays after holidays usually induce the over engineering part. I
agree, lets leave this part untouched for now.
Cheers,
Jussi
------------------------------
Date: Mon, 30 Dec 2019 15:10:39 +0200
From: Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH 3/3 v2] doc: Describe property array use with VPN
connection SetProperty D-Bus method
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel,
On 12/21/19 1:54 PM, Daniel Wagner wrote:
> In the case of the dictionary, the property name can be anything,
> correct? Maybe we should define it to something like SetProperties?
>
Yes, property name is not required. I think I remember leaving this open
for a reason: to get input on this matter. The property name could be
indeed used to make processing of the content more straightforward, but
I wasn't really sure about naming. Would plain "Properties" be enough?
Cheers,
Jussi
------------------------------
Date: Mon, 30 Dec 2019 15:04:25 +0100
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH 3/3 v2] doc: Describe property array use with VPN
connection SetProperty D-Bus method
To: Jussi Laakkonen <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
On 30.12.19 14:10, Jussi Laakkonen wrote:
> Yes, property name is not required. I think I remember leaving this open
> for a reason: to get input on this matter. The property name could be
> indeed used to make processing of the content more straightforward, but
> I wasn't really sure about naming. Would plain "Properties" be enough?
"Properties" is probably the best name for it. We call it so in the
documentation already, e.g. service-api.txt. This would be consistent
and as you say, we can simplify the parser.
------------------------------
Date: Mon, 30 Dec 2019 15:10:07 +0100
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH 2/3 v2] vpn-provider: Implement setting of
multiple VPN properties with one call
To: Jussi Laakkonen <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Jussi,
On 30.12.19 13:56, Jussi Laakkonen wrote:
> On 12/21/19 1:53 PM, Daniel Wagner wrote:
>> The missing concept in the Property API for changing multiple settings
>> at once leads to a bit strange things. Obvioulsy, it would be more
>> naturual to have single value set/get/clear/changed and the same thing
>> for multiple properties at once. Well, since this doesn't exist
>> neither for ConnMan nor for the freedesktop API let's go with this.
>>
>> Maybe we should bring this up at freedesktop and see what the
>> maintainers of the Property API think about it.
>>
>
> Having the methods for multiple properties would offer more flexibility,
> for the use of the method and in the underlying implementation. So yes,
> I'd agree that could be proposed in freedesktop as well.
>
> Specification [1] does, however, state, that there is a
> PropertiesChanged signal already in the D-Bus Properties interface
> (since version 0.14 in 2010) that can be used to indicate which
> properties were changed and which properties were invalidated (array of
> strings). So, the error reporting for the need of setting multiple
> properties at once (and clearing etc.) is already speficied. This may be
> good to implement in connman as well, when service.c would support
> setting multiple properties in one call.
The only thing we need to be careful, is not breaking existing users.
That means we still need to fire individual property changed signals.
> From the looks of it, PropertiesChanged is used only by the network
> manager compatiblity plugin (plugins/nmcompat.c) and gsupplicant.
> PropertiesChanged would not fit to all situations but to the ones where
> multiple properties are changed in any way.
IIRC, we needed it to emulate the NM interfaces. The 1.0 release of
ConnMan shipped without this API. In hindsight, it would have been
better to go with a PropertiesChanged...
Thanks,
Daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list -- [email protected]
To unsubscribe send an email to [email protected]
------------------------------
End of connman Digest, Vol 50, Issue 13
***************************************
