statedir compile time option - WIP #1400

Closed
wants to merge 19 commits into from

3 participants

@cdituri

Soliciting feedback / commentary / thoughts from those who know better; still very much a WIP and something I'm wading through.

Mainly useful to package maintainers... I personally need to see a state/ dir compile time option to finish packaging cfe for openwrt. For me an option like this would help isolate [frequent] writes away from flash and toward something more embedded friendly, like tmpfs/ramfs.

@tzz
tzz commented Feb 7, 2014

I think this is a good direction.

@basvandervlies

Count me in +1

@cdituri

Thanks guys. If anyone has suggestions or gotchas to look out for please let me know--particularly @basvandervlies since you recently went through the same exercise with inputdir and masterdir :-)

Going to keep this open for a little bit longer then close out until its ready.

@ediosyncratic ediosyncratic commented on an outdated diff Feb 10, 2014
cf-monitord/mon_network_sniffer.c
}
else
{
- snprintf(filename, CF_BUFSIZE - 1, "%s/state/cf_outgoing.%s", CFWORKDIR, TCPNAMES[i]);
+ snprintf(filename, CF_BUFSIZE - 1, "%s%ccf_outgoing.%s", GetStateDir(), FILE_SEPARATOR, TCPNAMES[i]);

(Pre-existing) The buffer sizes here should be CF_BUFSIZE, without the -1, assuming filename is actually declared with size CF_BUFSIZE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ediosyncratic ediosyncratic commented on an outdated diff Feb 10, 2014
@@ -145,6 +145,7 @@ AS_IF([test x"$enable_fhs" = xyes], [
INPUTDIR='default'
LOGDIR='${localstatedir}/lib/cfengine'
PIDDIR='${localstatedir}/lib/cfengine'
+ STATEDIR='${localstatedir}/lib/cfengine'

Why not (here and below) a trailing /state on the path ? This would keep things as they are by default, while opening up scope for the user to configure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ediosyncratic ediosyncratic commented on an outdated diff Feb 10, 2014
examples/measure_log.cf
@@ -7,7 +7,10 @@
#######################################################
#
-# Look for a file in /var/cfengine/state/line_counter_measure.log
+# Look for a file in $STATEDIR/line_counter_measure.log
+#
+# $STATEDIR is /var/cfengine/state unless overridden at
+# compile time.

This appears to be a lie, unless I've misunderstood the configure.ac change ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ediosyncratic ediosyncratic commented on an outdated diff Feb 10, 2014
libpromises/generic_agent.c
snprintf(vbuff, CF_BUFSIZE, "%s%creports%cvarious", CFWORKDIR, FILE_SEPARATOR, FILE_SEPARATOR);
MakeParentDirectory(vbuff, force);
+ /* CSD TODO: OOTB cf-agent bails in LogTotalCompliance with custom logdir
+ * since logdir is never created (previously with MakeParentDirectory()).
+ *
+ * Find better place for this and/or factor out all directory/file prep.
+ */
+ snprintf(vbuff, CF_BUFSIZE, "%s%c.", GetLogDir(), FILE_SEPARATOR);
+ MakeParentDirectory(vbuff, force);

The MakeParentDirectory calls above don't seem to need a /. on the end; why does this one ?
Without it, you could just use GetLogDir() in place of vbuff !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ediosyncratic ediosyncratic commented on an outdated diff Feb 10, 2014
libpromises/generic_agent.c
@@ -1339,15 +1358,16 @@ static void CheckWorkingDirectories(EvalContext *ctx)
chmod(CFWORKDIR, (mode_t) (statbuf.st_mode & ~022));
}
- snprintf(vbuff, CF_BUFSIZE, "%s%cstate%c.", CFWORKDIR, FILE_SEPARATOR, FILE_SEPARATOR);
+ /* CSD TODO: check for statedir here */
+ snprintf(vbuff, CF_BUFSIZE, "%s%c.", GetStateDir(), FILE_SEPARATOR);
MakeParentDirectory(vbuff, false);

Again, why the /. here ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ediosyncratic ediosyncratic commented on an outdated diff Feb 10, 2014
libpromises/generic_agent.c
if (stat(vbuff, &statbuf) == -1)
{
- snprintf(vbuff, CF_BUFSIZE, "%s%cstate%c.", CFWORKDIR, FILE_SEPARATOR, FILE_SEPARATOR);
+ /* CSD TODO: prob can eliminate this snprintf */
+ snprintf(vbuff, CF_BUFSIZE, "%s%c.", GetStateDir(), FILE_SEPARATOR);
MakeParentDirectory(vbuff, false);

(Pre-existing) Another /.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ediosyncratic ediosyncratic and 1 other commented on an outdated diff Feb 10, 2014
libpromises/known_dirs.c
@@ -170,6 +186,13 @@ const char *GetPidDir(void)
return piddir == NULL ? GetDefaultPidDir() : piddir;
}
+const char *GetStateDir(void)
+{
+ const char *statedir = getenv("CFENGINE_TEST_OVERRIDE_WORKDIR");

Surely a workdir-based env var needs a /state appended, if set and used ?
Should we be introducing a separate STATEDIR env var ?

@cdituri
cdituri added a note Feb 11, 2014

Surely a workdir-based env var needs a /state appended, if set and used ?

Thanks for pointing this out.

Should we be introducing a separate STATEDIR env var ?

According to 95ee02c one is enough... @jimis ?

Ah - yes - I hadn't noticed the TEST part of the name. I thought this was a mechanism for normal users to over-ride the configuration directories, in production. Ignore me :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ediosyncratic ediosyncratic commented on an outdated diff Feb 10, 2014
libpromises/locks.c
@@ -154,6 +154,7 @@ static void RemoveDates(char *s)
// Canonifies or blanks our times/dates for locks where there would be an explosion of state
+ /* CSD TODO: wtf? */

(Pre-existing) Indeed. We could at least use sizeof("literal") - 1 and move half the computational cost to compile-time.
Also: "Fri Oct 1 1:1:1 UTC 2010" looks like a valid date to me, and is shorter: are we sure s has always been generated by something that uses two-digit hh:mm:ss ? It's entirely possible there are good grounds to be sure, but they should probably be documented here !

... hmm ... and are there any time-zones whose abbreviations are shorter than three letters ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cdituri

@basvandervlies @ediosyncratic - thank you both for your review.

@basvandervlies

@cdituri configure has a default option:

  • --localstatedir=DIR modifiable single-machine data [PREFIX/var] Maybe make use of this option instead of using a new one? You can set the default to:
  • /var/cfengine/state
@cdituri

Almost complete just need to address a couple things in libpromises/dbm_api.c.

@basvandervlies - I ended up working off of your original suggestion to set STATEDIR to "default". Can be changed back to the original way, ie your later suggestion, if that's what the direction deems.

cdituri added some commits Feb 10, 2014
@cdituri cdituri new autoconf option to make state directory configurable: --with-stat…
…edir=<directory>
5f6f0a3
@cdituri cdituri Make use of preprocessor macros for static known_dirs.c functions 7b59e4b
@cdituri cdituri rename MAX_WORKDIR_LENGTH macro to more generic MAX_DIR_LENGTH ca79eb5
@cdituri cdituri add GetStateDir() and supporting functions c08a633
@cdituri cdituri make use of GetStateDir() and replace hardcoded state/ paths a719ba6
@cdituri cdituri RemoveDates():
- use sizeof on datetime string literal to move computation to compile-time.
- comment on uncertainty of two-digit hh:mm:ss and time-zones shorter
  than three letters.

Pointed out by Edward Welbourne
ef8e517
@cdituri cdituri LogTotalCompliance(): prevent cf-agent from bailing with custom logdir df5a5f5
@cdituri cdituri adjust examples/measure_log.cf comment to account for compile-time st…
…atedir
c752188
@cdituri cdituri MonNetworkInit(): use GetStateDir() and refactor 664f01e
@cdituri cdituri MonitorInitialize(): make use of GetStateDir() b9b95cb
@cdituri cdituri make bootstrap.c functions, and their usage, respect GetStateDir() 3e20f63
@cdituri cdituri Last of the needed GetStateDir() usage in generic_agent.c feb951f
@cdituri cdituri prep cf-agent self-diagnostics for --with-statedir compile-time option ac0deb9
@cdituri cdituri GenericAgentInitialize(): make sure custom {log,pid,state} directorie…
…s are created
4caf00d
@cdituri cdituri tests/unit/Makefile.am: added known_dirs.c to libdb libtool library s…
…ources
e03c41c
@cdituri cdituri Strip state/ from DB_PATHS[]; make all DBIdToPath(dir, dbid) calls us…
…e GetStateDir()
7bafe81
@cdituri cdituri tests/unit: use CFENGINE_TEST_OVERRIDE_WORKDIR in combination with Ge…
…tStateDir()
d63858c
@cdituri cdituri known_dirs.c: Collapse GetInputDir(), GetMasterDir(), GetStateDir() w…
…ith preprocessor
fe1ad9e
@cdituri cdituri tests/load: use CFENGINE_TEST_OVERRIDE_WORKDIR in combination with Ge…
…tStateDir()
d9de711
@cdituri

Closing to open new PR for review

@cdituri cdituri closed this Feb 15, 2014
@estenberg estenberg pushed a commit that referenced this pull request Jul 18, 2014
Eystein Måløy Stenberg improve error message, Zendesk #1400 b528237
@vohi vohi pushed a commit that referenced this pull request Jul 21, 2014
Eystein Måløy Stenberg improve error message, Zendesk #1400
(cherry picked from commit b528237)
32d6be6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment