diff mbox

[1/6] xenconsoled: introduce log file abstraction

Message ID 1465228781-22754-2-git-send-email-wei.liu2@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Liu June 6, 2016, 3:59 p.m. UTC
It will be used to handle hypervsior log and guest console log.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/console/daemon/logfile.c | 229 +++++++++++++++++++++++++++++++++++++++++
 tools/console/daemon/logfile.h |  57 ++++++++++
 2 files changed, 286 insertions(+)
 create mode 100644 tools/console/daemon/logfile.c
 create mode 100644 tools/console/daemon/logfile.h

Comments

Ian Jackson July 8, 2016, 4:50 p.m. UTC | #1
Wei Liu writes ("[PATCH 1/6] xenconsoled: introduce log file abstraction"):
> It will be used to handle hypervsior log and guest console log.

Thanks.

> diff --git a/tools/console/daemon/logfile.h b/tools/console/daemon/logfile.h
> new file mode 100644
> index 0000000..87f3c51
...
> +#define LOGFILE_MAX_BACKUP_NUM  5
> +#define LOGFILE_MAX_SIZE        (1024*128)

These should surely be configurable by the end-user.

> +/* Append only abstraction for log file */
> +struct logfile_entry {
> +	int fd;
> +	off_t pos;
> +	off_t len;
> +};

AFAICT these types could be defined inside logfile.c, and `struct
logfile' declared (but not defined) here in the header file.

That would make the abstraction clearer.

> diff --git a/tools/console/daemon/logfile.c b/tools/console/daemon/logfile.c
> new file mode 100644
> index 0000000..ad73f8e
> --- /dev/null
> +++ b/tools/console/daemon/logfile.c
...
> +static void logfile_entry_free(struct logfile_entry *entry)
> +{
> +	if (!entry) return;
> +
> +	if (entry->fd >= 0) close(entry->fd);
> +	free(entry);
> +}

I do wonder whether it's actually worth having a whole separate struct
for an `entry'.  Each `struct logfile' has zero or one `struct
logfile_entry's but most of the time exactly one.

Eliminating the separate struct logfile_entry and folding it into
struct logfile would save some memory allocation (and associated error
handling).

> +	entry = calloc(1, sizeof(*entry));
> +	if (!entry) goto err;

This pattern calls close(0) on error!  Either declare that fd==0 means
"none here" (which would be sane IMO) or initialise fd=-1.

> +	entry->fd = open(path, O_CREAT|O_APPEND|O_WRONLY|O_CLOEXEC, mode);

I think we had concluded that O_CLOEXEC wasn't sufficiently portable ?
It's not needed here because xenconsoled does not exec.

> +	entry->pos = lseek(entry->fd, 0, SEEK_END);
> +	if (entry->pos == (off_t)-1) {
> +		dolog(LOG_ERR, "Unable to determine current file offset in %s",
> +		      path);
> +		goto err;
> +	}
> +
> +	if (fstat(entry->fd, &sb) < 0) {
> +		dolog(LOG_ERR, "Unable to fstat current file %s", path);
> +		goto err;
> +	}

I'm not sure why you need to keep track of `len' and `pos'
separately.  AFAICT len is never used.

> +struct logfile *logfile_new(const char *path, mode_t mode)
> +{

Why does logfile_new take a mode for which all callers pass 644 ?
Do we anticipate logs with different permissions ?

> +		if (rename(this, next) < 0 && errno != ENOENT) {
> +			dolog(LOG_ERR, "Failed to rename %s to %s",
> +			      this, next);
> +			goto err;
> +		}

If LOGFILE_MAX_BACKUP_NUM becomes configurable, this loop will have to
become more sophisticated.  I think it may have to count up and then
down again.

> +static ssize_t write_all(int fd, const char* buf, size_t len)
> +{
...

This is a copy of an identical function in io.c.

> +ssize_t logfile_append(struct logfile *file, const char *buf,
> +		       size_t len)
> +{
> +	ssize_t ret = 0;
> +
> +	while (len) {
> +		struct logfile_entry *entry = file->entry;
> +		size_t towrite = len;
> +		bool rollover = false;
> +
> +		if (entry->pos > file->maxlen) {
> +			rollover = true;
> +			towrite = 0;
> +		} else if (entry->pos + towrite > file->maxlen) {
> +			towrite = file->maxlen - entry->pos;
> +		}
...
[and later]
> +		if ((entry->pos == file->maxlen && len) || rollover) {

This logic is rather tangled.  Why not

  maxwrite = file->maxlen - file->pos;
  if (towrite > maxwrite) {
      rollover = 1;
      towrite = maxwrite;
      if (towrite < 0)
          towrite = 0;
  }

and then later simply

    if (rollover) {

?

> +
> +		if (towrite) {
> +			if (write_all(entry->fd, buf, towrite) != towrite) {
> +				dolog(LOG_ERR, "Unable to write file %s",
> +				      file->basepath);
> +				return -1;
> +			}
> +
> +			len -= towrite;
> +			buf += towrite;
> +			ret += towrite;

I'm not a huge fan of this approach where several control variables
need to be kept in step.  You could save the beginning and end of the
buffer, and then you could do away with ret and no longer need to
modify len.

Ian.
diff mbox

Patch

diff --git a/tools/console/daemon/logfile.c b/tools/console/daemon/logfile.c
new file mode 100644
index 0000000..ad73f8e
--- /dev/null
+++ b/tools/console/daemon/logfile.c
@@ -0,0 +1,229 @@ 
+/*
+ *  Copyright (C) Citrix 2016
+ *  Author(s): Wei Liu <wei.liu2@citrix.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; under version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+#define _GNU_SOURCE
+
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "utils.h"
+#include "logfile.h"
+
+static void logfile_entry_free(struct logfile_entry *entry)
+{
+	if (!entry) return;
+
+	if (entry->fd >= 0) close(entry->fd);
+	free(entry);
+}
+
+void logfile_free(struct logfile *f)
+{
+	if (!f) return;
+
+	logfile_entry_free(f->entry);
+
+	free(f->basepath);
+	free(f);
+}
+
+static struct logfile_entry *logfile_entry_new(const char *path, mode_t mode)
+{
+	struct logfile_entry *entry;
+	struct stat sb;
+
+	entry = calloc(1, sizeof(*entry));
+	if (!entry) goto err;
+
+	entry->fd = open(path, O_CREAT|O_APPEND|O_WRONLY|O_CLOEXEC, mode);
+	if (entry->fd < 0) {
+		dolog(LOG_ERR, "Failed to open log file %s", path);
+		goto err;
+	}
+
+	entry->pos = lseek(entry->fd, 0, SEEK_END);
+	if (entry->pos == (off_t)-1) {
+		dolog(LOG_ERR, "Unable to determine current file offset in %s",
+		      path);
+		goto err;
+	}
+
+	if (fstat(entry->fd, &sb) < 0) {
+		dolog(LOG_ERR, "Unable to fstat current file %s", path);
+		goto err;
+	}
+
+	entry->len = sb.st_size;
+	return entry;
+
+err:
+	logfile_entry_free(entry);
+	return NULL;
+}
+
+struct logfile *logfile_new(const char *path, mode_t mode)
+{
+	struct logfile *logfile;
+
+	logfile = calloc(1, sizeof(*logfile));
+	if (!logfile) goto err;
+
+	logfile->basepath = strdup(path);
+	if (!logfile->basepath) {
+		dolog(LOG_ERR, "Failed to strdup %s", path);
+		goto err;
+	}
+
+	logfile->mode = mode;
+	logfile->maxlen = LOGFILE_MAX_SIZE;
+
+
+	logfile->entry = logfile_entry_new(logfile->basepath, mode);
+
+	if (!logfile->entry) goto err;
+
+	return logfile;
+err:
+	logfile_free(logfile);
+	return NULL;
+}
+
+static int logfile_rollover(struct logfile *file)
+{
+	struct logfile_entry *old = file->entry, *new = NULL;
+	char *this = NULL;
+	char *next = NULL;
+	unsigned i;
+
+	if (asprintf(&next, "%s.%u", file->basepath,
+		     LOGFILE_MAX_BACKUP_NUM) < 0) {
+		dolog(LOG_ERR, "Failed to asprintf %s.%u",
+		      file->basepath, LOGFILE_MAX_BACKUP_NUM);
+		goto err;
+	}
+
+	for (i = LOGFILE_MAX_BACKUP_NUM; 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;
+			}
+		}
+
+		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;
+	}
+
+	new = logfile_entry_new(file->basepath, file->mode);
+	if (!new) goto err;
+
+	file->entry = new;
+	logfile_entry_free(old);
+
+	return 0;
+err:
+	free(this);
+	free(next);
+	return -1;
+}
+
+static ssize_t write_all(int fd, const char* buf, size_t len)
+{
+	ssize_t written = 0;
+	while (len) {
+		ssize_t r = write(fd, buf, len);
+		if (r < 0 && errno == EINTR)
+			continue;
+		if (r < 0)
+			return r;
+		len -= r;
+		buf += r;
+		written += r;
+	}
+
+	return written;
+}
+
+ssize_t logfile_append(struct logfile *file, const char *buf,
+		       size_t len)
+{
+	ssize_t ret = 0;
+
+	while (len) {
+		struct logfile_entry *entry = file->entry;
+		size_t towrite = len;
+		bool rollover = false;
+
+		if (entry->pos > file->maxlen) {
+			rollover = true;
+			towrite = 0;
+		} else if (entry->pos + towrite > file->maxlen) {
+			towrite = file->maxlen - entry->pos;
+		}
+
+		if (towrite) {
+			if (write_all(entry->fd, buf, towrite) != towrite) {
+				dolog(LOG_ERR, "Unable to write file %s",
+				      file->basepath);
+				return -1;
+			}
+
+			len -= towrite;
+			buf += towrite;
+			ret += towrite;
+			entry->pos += towrite;
+			entry->len += towrite;
+		}
+
+		if ((entry->pos == file->maxlen && len) || rollover) {
+			dolog(LOG_DEBUG, "Roll over %s (maxlen %zu)",
+			      file->basepath, file->maxlen);
+			if (logfile_rollover(file))
+				return -1;
+		}
+	}
+
+	return ret;
+}
+
+
+/*
+ * Local variables:
+ *  c-file-style: "linux"
+ *  indent-tabs-mode: t
+ *  c-indent-level: 8
+ *  c-basic-offset: 8
+ *  tab-width: 8
+ * End:
+ */
diff --git a/tools/console/daemon/logfile.h b/tools/console/daemon/logfile.h
new file mode 100644
index 0000000..87f3c51
--- /dev/null
+++ b/tools/console/daemon/logfile.h
@@ -0,0 +1,57 @@ 
+/*
+ *  Copyright (C) Citrix 2016
+ *  Author(s): Wei Liu <wei.liu2@citrix.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; under version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef XENCONSOLED_LOGFILE_H
+#define XENCONSOLED_LOGFILE_H
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#define LOGFILE_MAX_BACKUP_NUM  5
+#define LOGFILE_MAX_SIZE        (1024*128)
+
+/* Append only abstraction for log file */
+struct logfile_entry {
+	int fd;
+	off_t pos;
+	off_t len;
+};
+
+struct logfile {
+	char *basepath;
+	mode_t mode;
+	off_t maxlen;
+	struct logfile_entry *entry;
+};
+
+struct logfile *logfile_new(const char *path, mode_t mode);
+void logfile_free(struct logfile *f);
+ssize_t logfile_append(struct logfile *file, const char *buf,
+		       size_t len);
+
+#endif	/* XENCONSOLED_LOGFILE_H */
+
+/*
+ * Local variables:
+ *  c-file-style: "linux"
+ *  indent-tabs-mode: t
+ *  c-indent-level: 8
+ *  c-basic-offset: 8
+ *  tab-width: 8
+ * End:
+ */