Page MenuHomePhabricator

elm_config: fix elm_config_save not to wake idle processes up
Needs RevisionPublic

Authored by akanad on Nov 23 2018, 3:51 AM.

Details

Summary

there is a eio_monitor which is tracking on config directory.
and that is the reason why idle processes get waken up once some process call elm_config_save().
this patch fix the function not to wake idle processes up as it was before.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
akanad created this revision.Nov 23 2018, 3:51 AM
akanad requested review of this revision.Nov 23 2018, 3:51 AM
akanad updated this revision to Diff 17601.Nov 23 2018, 7:47 PM

removes a tmp file after copying

akanad updated this revision to Diff 17626.Nov 26 2018, 8:49 PM

Makes empty .cfg files in advance to make eio_monitor be able to track them.

@cedric
if you don't mind, would you please review this patch?

cedric accepted this revision.Nov 28 2018, 4:07 PM

Seems to make sense. Would be neat to have an easy way to test the previous and new behavior.

This revision is now accepted and ready to land.Nov 28 2018, 4:07 PM
This revision was automatically updated to reflect the committed changes.
bu5hm4n reopened this revision.Thu, Feb 7, 9:06 AM
bu5hm4n added subscribers: WhiskyKilo, bu5hm4n.

Okay, stop.

This commit breaks my config ... for the second time now ?

Whenever there is a call to _config_profile_change_delay_cb, when no .base.flush.cfg file does exist, then the empty value from the .base.flush.cfg is loaded and stuffed into the real config, which basically erases the config of a user ... ...

Further more, the commit messages tells that idle processes are woken up because there config does change, yes that is true, and i pretty much see no way arround it, we need that in order to sync up. What you *can* do, is to stop the monitor before the process goes idle, and once the process comes back, you can check for changes.

A side note: Please never ever create empty files which are used to read in some config, this is really asking for trouble, as every single time, the state there is empty you erase the complete config of a user. Please be a little bit more carefull with such patches!

@WhiskyKilo can you take another look ? Or redo this patch ?

This revision is now accepted and ready to land.Thu, Feb 7, 9:06 AM
bu5hm4n requested changes to this revision.Thu, Feb 7, 9:08 AM
This revision now requires changes to proceed.Thu, Feb 7, 9:08 AM

Okay, stop.

This commit breaks my config ... for the second time now ?

Whenever there is a call to _config_profile_change_delay_cb, when no .base.flush.cfg file does exist, then the empty value from the .base.flush.cfg is loaded and stuffed into the real config, which basically erases the config of a user ... ...

I'm on looking at it.

Further more, the commit messages tells that idle processes are woken up because there config does change, yes that is true, and i pretty much see no way arround it, we need that in order to sync up. What you *can* do, is to stop the monitor before the process goes idle, and once the process comes back, you can check for changes.

First.
Compared to elm_config_all_flush(), elm_config_save() didn't wake up idle processes before the commit(8589d1d2ab2). but without this(D7354) patch, both APIs wake up all processes. that's the main reson why I wrote this patch.

Second.
I don't really agree that "removing file monitor" because "removing file monitor" mean "removing a way to wake up processes"

A side note: Please never ever create empty files which are used to read in some config, this is really asking for trouble, as every single time, the state there is empty you erase the complete config of a user. Please be a little bit more carefull with such patches!

I'm going to write a patch to make a file with information soon.

@WhiskyKilo can you take another look ? Or redo this patch ?

I can see what you are trying to achive, and even though this is documented, i don't think this is a good idea to do like this. There are a bit nicer ways to do that, and further more, i don't think this is a good idea overall.

Anyways, this is deleting my config like 2-3 times a day, every time i am testing something with elementary_test or elementary_config. Can we go with a revert until then?

A mock of how we could achive something simular:

Our config file monitors a file in the config directory called "config.cfg.monitor" when flush all is done, something is written to this file even just 1 single byte or so, however, all processes are woken up. The save is not doing this, and thus we achived what we wanted to. Additionally, profile changes are accepted by any app, even if it is just called by the save method, so @raster concerns in this commit above are addressed.

then woud you please revert this? I will write another one as you said.