From e3dea0043973faf42f7756d840bc55aa8f143eb1 Mon Sep 17 00:00:00 2001 From: William Brown Date: Wed, 15 Nov 2017 13:44:02 +1000 Subject: [PATCH] Ticket 49298 - Correct error codes with config restore. Bug Description: The piece of code uses 0 as an error - not 1, and in some cases did not even check the codes or use the correct logic. Fix Description: Cleanup dse_check_file to better check the content of files and communicate issues to the admin. Correct slapd_bootstrap_config to correctly handle the cases of removal and restore. https://pagure.io/389-ds-base/issue/49298 Author: wibrown Review by: mreynoolds & spichugi Signed-off-by: Mark Reynolds (cherry picked from commit 75e55e26579955adf058e8adcba9a28779583b7b) --- .../suites/config/removed_config_49298_test.py | 81 ++++++++++++++++++++++ ldap/servers/slapd/config.c | 15 ++-- ldap/servers/slapd/dse.c | 42 ++++++++--- 3 files changed, 119 insertions(+), 19 deletions(-) create mode 100644 dirsrvtests/tests/suites/config/removed_config_49298_test.py diff --git a/dirsrvtests/tests/suites/config/removed_config_49298_test.py b/dirsrvtests/tests/suites/config/removed_config_49298_test.py new file mode 100644 index 000000000..e65236924 --- /dev/null +++ b/dirsrvtests/tests/suites/config/removed_config_49298_test.py @@ -0,0 +1,81 @@ +# --- BEGIN COPYRIGHT BLOCK --- +# Copyright (C) 2017 Red Hat, Inc. +# All rights reserved. +# +# License: GPL (version 3 or any later version). +# See LICENSE for details. +# --- END COPYRIGHT BLOCK --- +# +import pytest +import os +import logging +import subprocess + +from lib389.topologies import topology_st as topo + +DEBUGGING = os.getenv("DEBUGGING", default=False) +if DEBUGGING: + logging.getLogger(__name__).setLevel(logging.DEBUG) +else: + logging.getLogger(__name__).setLevel(logging.INFO) +log = logging.getLogger(__name__) + +def test_restore_config(topo): + """ + Check that if a dse.ldif and backup are removed, that the server still starts. + + :id: e1c38fa7-30bc-46f2-a934-f8336f387581 + :setup: Standalone instance + :steps: + 1. Stop the instance + 2. Delete 'dse.ldif' + 3. Start the instance + :expectedresults: + 1. Steps 1 and 2 succeed. + 2. Server will succeed to start with restored cfg. + """ + topo.standalone.stop() + + dse_path = topo.standalone.get_config_dir() + + log.info(dse_path) + + for i in ('dse.ldif', 'dse.ldif.startOK'): + p = os.path.join(dse_path, i) + os.remove(p) + + # This will pass. + topo.standalone.start() + +def test_removed_config(topo): + """ + Check that if a dse.ldif and backup are removed, that the server + exits better than "segfault". + + :id: b45272d1-c197-473e-872f-07257fcb2ec0 + :setup: Standalone instance + :steps: + 1. Stop the instance + 2. Delete 'dse.ldif', 'dse.ldif.bak', 'dse.ldif.startOK' + 3. Start the instance + :expectedresults: + 1. Steps 1 and 2 succeed. + 2. Server will fail to start, but will not crash. + """ + topo.standalone.stop() + + dse_path = topo.standalone.get_config_dir() + + log.info(dse_path) + + for i in ('dse.ldif', 'dse.ldif.bak', 'dse.ldif.startOK'): + p = os.path.join(dse_path, i) + os.remove(p) + + # We actually can't check the log output, because it can't read dse.ldif, + # don't know where to write it yet! All we want is the server fail to + # start here, rather than infinite run + segfault. + with pytest.raises(subprocess.CalledProcessError): + topo.standalone.start() + + diff --git a/ldap/servers/slapd/config.c b/ldap/servers/slapd/config.c index afe07df84..c8d57e747 100644 --- a/ldap/servers/slapd/config.c +++ b/ldap/servers/slapd/config.c @@ -121,14 +121,13 @@ slapd_bootstrap_config(const char *configdir) "Passed null config directory\n"); return rc; /* Fail */ } - PR_snprintf(configfile, sizeof(configfile), "%s/%s", configdir, - CONFIG_FILENAME); - PR_snprintf(tmpfile, sizeof(tmpfile), "%s/%s.tmp", configdir, - CONFIG_FILENAME); - if ((rc = dse_check_file(configfile, tmpfile)) == 0) { - PR_snprintf(tmpfile, sizeof(tmpfile), "%s/%s.bak", configdir, - CONFIG_FILENAME); - rc = dse_check_file(configfile, tmpfile); + PR_snprintf(configfile, sizeof(configfile), "%s/%s", configdir, CONFIG_FILENAME); + PR_snprintf(tmpfile, sizeof(tmpfile), "%s/%s.bak", configdir, CONFIG_FILENAME); + rc = dse_check_file(configfile, tmpfile); + if (rc == 0) { + /* EVERYTHING IS GOING WRONG, ARRGHHHHHH */ + slapi_log_err(SLAPI_LOG_ERR, "slapd_bootstrap_config", "No valid configurations can be accessed! You must restore %s from backup!\n", configfile); + return 0; } if ((rc = PR_GetFileInfo64(configfile, &prfinfo)) != PR_SUCCESS) { diff --git a/ldap/servers/slapd/dse.c b/ldap/servers/slapd/dse.c index 420248c24..653009f53 100644 --- a/ldap/servers/slapd/dse.c +++ b/ldap/servers/slapd/dse.c @@ -609,29 +609,49 @@ dse_check_file(char *filename, char *backupname) if (PR_GetFileInfo64(filename, &prfinfo) == PR_SUCCESS) { if (prfinfo.size > 0) { - return (1); + /* File exists and has content. */ + return 1; } else { + slapi_log_err(SLAPI_LOG_INFO, "dse_check_file", + "The config %s has zero length. Attempting restore ... \n", filename, rc); rc = PR_Delete(filename); } + } else { + slapi_log_err(SLAPI_LOG_INFO, "dse_check_file", + "The config %s can not be accessed. Attempting restore ... (reason: %d)\n", filename, rc); } if (backupname) { + + if (PR_GetFileInfo64(backupname, &prfinfo) != PR_SUCCESS) { + slapi_log_err(SLAPI_LOG_INFO, "dse_check_file", + "The backup %s can not be accessed. Check it exists and permissions.\n", backupname); + return 0; + } + + if (prfinfo.size <= 0) { + slapi_log_err(SLAPI_LOG_ERR, "dse_check_file", + "The backup file %s has zero length, refusing to restore it.\n", backupname); + return 0; + } + rc = PR_Rename(backupname, filename); - } else { - return (0); - } + if (rc != PR_SUCCESS) { + slapi_log_err(SLAPI_LOG_INFO, "dse_check_file", + "The configuration file %s was NOT able to be restored from %s, error %d\n", filename, backupname, rc); + return 0; + } - if (PR_GetFileInfo64(filename, &prfinfo) == PR_SUCCESS && prfinfo.size > 0) { slapi_log_err(SLAPI_LOG_INFO, "dse_check_file", - "The configuration file %s was restored from backup %s\n", filename, backupname); - return (1); + "The configuration file %s was restored from backup %s\n", filename, backupname); + return 1; + } else { - slapi_log_err(SLAPI_LOG_ERR, "dse_check_file", - "The configuration file %s was not restored from backup %s, error %d\n", - filename, backupname, rc); - return (0); + slapi_log_err(SLAPI_LOG_INFO, "dse_check_file", "No backup filename provided.\n"); + return 0; } } + static int dse_read_one_file(struct dse *pdse, const char *filename, Slapi_PBlock *pb, int primary_file) { -- 2.13.6