[6/6] xenconsoled: handle --log-backups 0 in logfile_rollover

Commit Message

Wei Liu June 6, 2016, 3:59 p.m. UTC

We now allow user to configure the number of backups to keep. We need to
handle when the number is set to 0.

Check if number of backup is 0. If so, just unlink the file. Move the
rotation to `else' branch so that we skip it altogether.

Signed-off-by: Wei Liu <[e-mail]>
---
 tools/console/daemon/logfile.c | 63 +++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 26 deletions(-)

Comments

Ian Jackson July 8, 2016, 5:03 p.m. UTC | #1

Wei Liu writes ("[PATCH 6/6] xenconsoled: handle --log-backups 0 in logfile_rollover"):
> We now allow user to configure the number of backups to keep. We need to
> handle when the number is set to 0.
> 
> Check if number of backup is 0. If so, just unlink the file. Move the
> rotation to `else' branch so that we skip it altogether.

I would prefer to review one algorithm correct from the start...

Thanks,
Ian.

Patch

diff --git a/tools/console/daemon/logfile.c b/tools/console/daemon/logfile.c
index 3b95f84..fd29ba5 100644
--- a/tools/console/daemon/logfile.c
+++ b/tools/console/daemon/logfile.c
@@ -114,38 +114,49 @@  static int logfile_rollover(struct logfile *file)
 	char *next = NULL;
 	unsigned i;
 
-	if (asprintf(&next, "%s.%u", file->basepath,
-		     log_backups) < 0) {
-		dolog(LOG_ERR, "Failed to asprintf %s.%u",
-		      file->basepath, log_backups);
-		goto err;
-	}
+	if (log_backups == 0) {
+		if (unlink(file->basepath) < 0 &&
+		    errno != ENOENT) {
+			dolog(LOG_ERR, "Failed to unlink %s",
+			      file->basepath);
+			goto err;
+		}
+	} else {
+		if (asprintf(&next, "%s.%u", file->basepath,
+			     log_backups) < 0) {
+			dolog(LOG_ERR, "Failed to asprintf %s.%u",
+			      file->basepath, log_backups);
+			goto err;
+		}
 
-	for (i = log_backups; i > 0; i--) {
-		if (i == 1) {
-			this = strdup(file->basepath);
-			if (!this) {
-				dolog(LOG_ERR, "Failed to strdup %s",
-				      file->basepath);
-				goto err;
+		for (i = log_backups; i > 0; i--) {
+			if (i == 1) {
+				this = strdup(file->basepath);
+				if (!this) {
+					dolog(LOG_ERR, "Failed to strdup %s",
+					      file->basepath);
+					goto err;
+				}
+			} else {
+				if (asprintf(&this, "%s.%u", file->basepath,
+					     i-1) < 0) {
+					dolog(LOG_ERR,
+					      "Failed to asprintf %s.%u",
+					      file->basepath, i-1);
+					goto err;
+				}
 			}
-		} else {
-			if (asprintf(&this, "%s.%u", file->basepath, i-1) < 0) {
-				dolog(LOG_ERR, "Failed to asprintf %s.%u",
-				      file->basepath, i-1);
+
+			if (rename(this, next) < 0 && errno != ENOENT) {
+				dolog(LOG_ERR, "Failed to rename %s to %s",
+				      this, next);
 				goto err;
 			}
-		}
 
-		if (rename(this, next) < 0 && errno != ENOENT) {
-			dolog(LOG_ERR, "Failed to rename %s to %s",
-			      this, next);
-			goto err;
+			free(next);
+			next = this;
+			this = NULL;
 		}
-
-		free(next);
-		next = this;
-		this = NULL;
 	}
 
 	new = logfile_entry_new(file->basepath, file->mode);