Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make masterdir configurable via configure option: --with-masterdir=<directory> #1260

Merged
merged 4 commits into from
Jan 13, 2014

Conversation

basvandervlies
Copy link
Contributor

This is being discussed on cfengine-debian mailing list. default value: sys.workdir/masterfiles


const char *GetMasterDir(void)
{
const char *masterdir = getenv("CFENGINE_TEST_OVERRIDE_WORKDIR");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is workdir, not masterdir; if used, it needs a path-separator + "masterfiles" suffix, I suppose.
Alternatively, we also need a CFENGINE_TEST_OVERRIDE_MASTERDIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct we have to add 'masterfiles'. Thanks

@ediosyncratic
Copy link
Contributor

One problem I see: if someone changes the workdir, via any of the available controls (env, cmd line, etc.), the prior code used to automatically change masterdir to be workdir/masterfiles; unless I've missed something, they'll now need to explicitly also over-ride masterdir, or they'll be left with the default masterdir, not under their adjusted workdir (as, I think, they used to get). This may be desirable in some cases but is not backwards-compatible (if I've understood correctly - and I'm not entirely familiar with the global variable set-up, so this isn't a given).

@basvandervlies
Copy link
Contributor Author

The getenv stuff is only for testing purposes. It is used to set cfengine workdir to a test environment (make test). workdir is set to this value and masterfiles is also adjusted to be workdir/masterfiles. This is the default behavior.

Masterfiles is only overridden by an configure option (hard coded) or via the getenv method for testing purposes.

@ediosyncratic
Copy link
Contributor

Ah - OK - I had indeed misunderstood. I was thinking run-time configuration, this is ./configure-time, so my worry was entirely spurious :-)

@tzz
Copy link
Contributor

tzz commented Jan 9, 2014

I'd like the sys.inputdir to be configurable the same way as sys.masterdir please. Can that be part of this PR?

@basvandervlies
Copy link
Contributor Author

@tzz no problem. Or must i make a new one? There are 2 separate things. Due this change we have to support variable expansion at the server side (cf-serverd). relative paths do not work anymore, see https://cfengine.com/dev/issues/3697

@tzz
Copy link
Contributor

tzz commented Jan 9, 2014

I think it's OK to put a new commit into this PR.

Relative paths never worked :) I think the variable expansion is the way to go to solve that issue, but regardless we should allow @sys.masterdir@ and @sys.inputdir@ to be configurable at compilation.

@basvandervlies
Copy link
Contributor Author

On 9 jan. 2014, at 15:22, Ted Zlatanov notifications@github.com wrote:

I think it's OK to put a new commit into this PR.

Relative paths never worked :) I think the variable expansion is the way to go to solve that issue, but regardless we should allow @sys.masterdir@ and @sys.inputdir@ to be configurable at compilation.

You are right i noticed this when i changed the sys.masterdir definition.


SURFsara has a new telephone number: +31 20 800 1300.

Bas van der Vlies
| Operations, Support & Development | SURFsara | Science Park 140 | 1098 XG Amsterdam
| T +31 (0) 20 800 1300 | bas.vandervlies@surfsara.nl | www.surfsara.nl |

 * ./configure --with-inputdir=<directory> (default: sys.workdir/inputs)
@basvandervlies
Copy link
Contributor Author

I just realized that i have to adjust some files to make use of the new functions GetInputDir() and GetMasterDir(). I will do it today

 * GetInputDir() for inputs directory (sys.inputdir)
 * GetMasterDir()  for masterfiles directory (sys.masterdir)
@tzz
Copy link
Contributor

tzz commented Jan 13, 2014

This looks good to me to be merged. masterfiles.git/bootstrap/failsafe.cf also needs to be updated.

tzz added a commit that referenced this pull request Jan 13, 2014
make masterdir and inputdir configurable via configure options
@tzz tzz merged commit 52a965b into cfengine:master Jan 13, 2014
@basvandervlies
Copy link
Contributor Author

@tzz i will do it when this is accepted. Mark, Brian, Martin and me agreed on this options. We must make sure that we do no compromise the security model.

@kacf
Copy link
Contributor

kacf commented Jan 14, 2014

These changes were reverted in #1287, for the reasons described there.

{
char workbuf[CF_BUFSIZE];
snprintf(workbuf, CF_BUFSIZE, "%s%cmasterfiles", workdir, FILE_SEPARATOR);
MapName(workbuf);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trouble. This returns workbuf, which is a local variable of this function ... except that you forgot the word "return" which is required here, so this function actually returns a random pointer, which is probably a bad thing !
If you make workbuf static this can be salvaged. (If you return a malloc()ed string, you avoid the problems with workbuf but you'll leak unless callers are required to free() this function's return, in which case the else clause's return also needs a strdup() call on it.)
GetInputDir() has exactly the same problems.

The compiler warns about the lack of return statement here: please check for new compiler warnings (e.g. ones mentioning your new functions) in future ! I'll patch this by making the arrays static.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! Oversight at my side. Maybe this causing that the build fails and the pull request is reverted

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't cause compilation to fail, and it won't have any effect unless the environment variable is set; since you just added it here (and I see no test that sets it), I don't imagine it has any other way to break the build. See #1287 for the actual reason for revert !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... oh, and I won't be patching this, given that it's been reverted !

@basvandervlies
Copy link
Contributor Author

Issued a new request: #1293

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants