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
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
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
>> 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.