[libvirt] [PATCH 2/2] Introduce safewrite_str
John Ferlan
jferlan at redhat.com
Thu Feb 18 12:40:12 UTC 2016
On 02/12/2016 08:59 AM, Ján Tomko wrote:
> Just like safewrite, but calls strlen first to figure out
> the length of the string.
> ---
> src/conf/virchrdev.c | 2 +-
> src/libvirt_private.syms | 1 +
> src/lxc/lxc_process.c | 4 ++--
> src/network/leaseshelper.c | 2 +-
> src/openvz/openvz_conf.c | 15 ++++++---------
> src/qemu/qemu_domain.c | 2 +-
> src/util/vircommand.c | 4 ++--
> src/util/virfile.c | 9 ++++++++-
> src/util/virfile.h | 1 +
> src/util/virlog.c | 6 +++---
> src/util/virpidfile.c | 4 ++--
> src/util/virxml.c | 17 ++++++-----------
> tools/vsh.c | 4 +---
> 13 files changed, 35 insertions(+), 36 deletions(-)
>
Conflicted about this one - the difference between the two appears to be
'safewrite' will write a some length of a string (e.g. a counted length)
while safewrite_str writes the entire string. To make things more
interesting some safewrite calls use sizeof instead of strlen, but in
reality are just writing everything in the buffer.
So, perhaps 'safewrite_all' or 'safewrite_full' better describes the new
functionality?
I also started looking at some of the other safewrite calls to see if
any more fell into the bucket of write everything and few more jumped
out at me. Some used sizeof instead of strlen and some uses of strlen
were better hidden, such as:
* secretSaveDef vectors through replaceFile
* update_include_file
The other thing that I noted is error checking is not consistent. While
checking < 0 does ensure that the write() call didn't fail, what if the
number of bytes written != the number of bytes we tried to write?
So maybe this is a good opportunity to make all this consistent for all
callers.
BTW: saferead is in a similar situation...
> diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c
> index 701b326..1fe1631 100644
> --- a/src/conf/virchrdev.c
> +++ b/src/conf/virchrdev.c
> @@ -162,7 +162,7 @@ static int virChrdevLockFileCreate(const char *dev)
> }
>
> /* write the pid to the file */
> - if (safewrite(lockfd, pidStr, strlen(pidStr)) < 0) {
> + if (safewrite_str(lockfd, pidStr) < 0) {
> virReportSystemError(errno,
> _("Couldn't write to lock file for "
> "device '%s' in path '%s'"),
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 4cfaed5..c17c5e7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1431,6 +1431,7 @@ virEventPollUpdateTimeout;
> # util/virfile.h
> saferead;
> safewrite;
> +safewrite_str;
> safezero;
> virBuildPathInternal;
> virDirCreate;
> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> index 50bee48..1679411 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -1407,8 +1407,8 @@ int virLXCProcessStart(virConnectPtr conn,
> /* Log timestamp */
> if ((timestamp = virTimeStringNow()) == NULL)
> goto cleanup;
> - if (safewrite(logfd, timestamp, strlen(timestamp)) < 0 ||
> - safewrite(logfd, START_POSTFIX, strlen(START_POSTFIX)) < 0) {
> + if (safewrite_str(logfd, timestamp) < 0 ||
> + safewrite_str(logfd, START_POSTFIX) < 0) {
> VIR_WARN("Unable to write timestamp to logfile: %s",
> virStrerror(errno, ebuf, sizeof(ebuf)));
> }
> diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
> index 55ddd58..2414a2f 100644
> --- a/src/network/leaseshelper.c
> +++ b/src/network/leaseshelper.c
> @@ -84,7 +84,7 @@ customLeaseRewriteFile(int fd, void *opaque)
> {
> char **data = opaque;
>
> - if (safewrite(fd, *data, strlen(*data)) < 0)
> + if (safewrite_str(fd, *data) < 0)
> return -1;
>
> return 0;
> diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
> index e32dd6f..edd989e 100644
> --- a/src/openvz/openvz_conf.c
> +++ b/src/openvz/openvz_conf.c
> @@ -673,16 +673,15 @@ openvzWriteConfigParam(const char * conf_file, const char *param, const char *va
> break;
>
> if (!(STRPREFIX(line, param) && line[strlen(param)] == '=')) {
> - if (safewrite(temp_fd, line, strlen(line)) !=
> - strlen(line))
> + if (safewrite_str(temp_fd, line) < 0)
> goto error;
> }
> }
>
> - if (safewrite(temp_fd, param, strlen(param)) < 0 ||
> - safewrite(temp_fd, "=\"", 2) < 0 ||
> - safewrite(temp_fd, value, strlen(value)) < 0 ||
> - safewrite(temp_fd, "\"\n", 2) < 0)
> + if (safewrite_str(temp_fd, param) < 0 ||
> + safewrite_str(temp_fd, "=\"") < 0 ||
> + safewrite_str(temp_fd, value) < 0 ||
> + safewrite_str(temp_fd, "\"\n") < 0)
> goto error;
>
> if (VIR_FCLOSE(fp) < 0)
> @@ -801,7 +800,6 @@ openvz_copyfile(char* from_path, char* to_path)
> size_t line_size = 0;
> FILE *fp;
> int copy_fd;
> - int bytes_read;
>
> fp = fopen(from_path, "r");
> if (fp == NULL)
> @@ -816,8 +814,7 @@ openvz_copyfile(char* from_path, char* to_path)
> if (getline(&line, &line_size, fp) <= 0)
> break;
>
> - bytes_read = strlen(line);
> - if (safewrite(copy_fd, line, bytes_read) != bytes_read)
> + if (safewrite_str(copy_fd, line) < 0)
> goto error;
> }
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 495c76b..698ddfd 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2481,7 +2481,7 @@ int qemuDomainLogContextWrite(qemuDomainLogContextPtr ctxt,
> _("Unable to seek to end of domain logfile"));
> goto cleanup;
> }
> - if (safewrite(ctxt->writefd, message, strlen(message)) < 0) {
> + if (safewrite_str(ctxt->writefd, message) < 0) {
> virReportSystemError(errno, "%s",
> _("Unable to write to domain logfile"));
> goto cleanup;
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index fe7bf34..60baca0 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -1902,13 +1902,13 @@ virCommandWriteArgLog(virCommandPtr cmd, int logfd)
> return;
>
> for (i = 0; i < cmd->nenv; i++) {
> - if (safewrite(logfd, cmd->env[i], strlen(cmd->env[i])) < 0)
> + if (safewrite_str(logfd, cmd->env[i]) < 0)
> ioError = errno;
> if (safewrite(logfd, " ", 1) < 0)
> ioError = errno;
> }
> for (i = 0; i < cmd->nargs; i++) {
> - if (safewrite(logfd, cmd->args[i], strlen(cmd->args[i])) < 0)
> + if (safewrite_str(logfd, cmd->args[i]) < 0)
> ioError = errno;
> if (safewrite(logfd, i == cmd->nargs - 1 ? "\n" : " ", 1) < 0)
> ioError = errno;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f45e18f..c4fe10e 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -1066,6 +1066,13 @@ safewrite(int fd, const void *buf, size_t count)
> return nwritten;
> }
>
Could use an intro comment about usage... As well as a returns
statement... Since neither generates an error message it's up to the
caller to handle that... Not all callers do that today.
> +ssize_t
> +safewrite_str(int fd, const char *buf)
> +{
> + size_t len = strlen(buf);
> + return safewrite(fd, buf, len);
So this only returns failure if write() fails; however:
size_t written = safewrite(fd, buf, len);
if (written != len) /* different variable types... */
return -1;
return written;
John
> +}
> +
> #ifdef HAVE_POSIX_FALLOCATE
> static int
> safezero_posix_fallocate(int fd, off_t offset, off_t len)
> @@ -1418,7 +1425,7 @@ virFileWriteStr(const char *path, const char *str, mode_t mode)
> if (fd == -1)
> return -1;
>
> - if (safewrite(fd, str, strlen(str)) < 0) {
> + if (safewrite_str(fd, str) < 0) {
> VIR_FORCE_CLOSE(fd);
> return -1;
> }
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 312f226..12df539 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -41,6 +41,7 @@ typedef enum {
> ssize_t saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK;
> ssize_t safewrite(int fd, const void *buf, size_t count)
> ATTRIBUTE_RETURN_CHECK;
> +ssize_t safewrite_str(int fd, const char *buf) ATTRIBUTE_RETURN_CHECK;
> int safezero(int fd, off_t offset, off_t len)
> ATTRIBUTE_RETURN_CHECK;
>
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index b8398d1..3015d1a 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -681,9 +681,9 @@ virLogStackTraceToFd(int fd)
> size = backtrace(array, ARRAY_CARDINALITY(array));
> if (size) {
> backtrace_symbols_fd(array + STRIP_DEPTH, size - STRIP_DEPTH, fd);
> - ignore_value(safewrite(fd, "\n", 1));
> + ignore_value(safewrite_str(fd, "\n"));
> } else if (!doneWarning) {
> - ignore_value(safewrite(fd, msg, strlen(msg)));
> + ignore_value(safewrite_str(fd, msg));
> doneWarning = true;
> }
> #undef STRIP_DEPTH
> @@ -711,7 +711,7 @@ virLogOutputToFd(virLogSourcePtr source ATTRIBUTE_UNUSED,
> if (virAsprintfQuiet(&msg, "%s: %s", timestamp, str) < 0)
> return;
>
> - ignore_value(safewrite(fd, msg, strlen(msg)));
> + ignore_value(safewrite_str(fd, msg));
> VIR_FREE(msg);
>
> if (flags & VIR_LOG_STACK_TRACE)
> diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
> index 58ab29f..d000fc2 100644
> --- a/src/util/virpidfile.c
> +++ b/src/util/virpidfile.c
> @@ -77,7 +77,7 @@ int virPidFileWritePath(const char *pidfile,
>
> snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);
>
> - if (safewrite(fd, pidstr, strlen(pidstr)) < 0) {
> + if (safewrite_str(fd, pidstr) < 0) {
> rc = -errno;
> VIR_FORCE_CLOSE(fd);
> goto cleanup;
> @@ -446,7 +446,7 @@ int virPidFileAcquirePath(const char *path,
>
> snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);
>
> - if (safewrite(fd, pidstr, strlen(pidstr)) < 0) {
> + if (safewrite_str(fd, pidstr) < 0) {
> virReportSystemError(errno,
> _("Failed to write to pid file '%s'"),
> path);
> diff --git a/src/util/virxml.c b/src/util/virxml.c
> index 489bad8..aeed9ce 100644
> --- a/src/util/virxml.c
> +++ b/src/util/virxml.c
> @@ -796,7 +796,6 @@ static int virXMLEmitWarning(int fd,
> const char *name,
> const char *cmd)
> {
> - size_t len;
> const char *prologue =
> "<!--\n"
> "WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE\n"
> @@ -812,25 +811,21 @@ static int virXMLEmitWarning(int fd,
> return -1;
> }
>
> - len = strlen(prologue);
> - if (safewrite(fd, prologue, len) != len)
> + if (safewrite_str(fd, prologue) < 0)
> return -1;
>
> - len = strlen(cmd);
> - if (safewrite(fd, cmd, len) != len)
> + if (safewrite_str(fd, cmd) < 0)
> return -1;
>
> if (name) {
> - if (safewrite(fd, " ", 1) != 1)
> + if (safewrite_str(fd, " ") < 0)
> return -1;
>
> - len = strlen(name);
> - if (safewrite(fd, name, len) != len)
> + if (safewrite_str(fd, name) < 0)
> return -1;
> }
>
> - len = strlen(epilogue);
> - if (safewrite(fd, epilogue, len) != len)
> + if (safewrite_str(fd, epilogue) < 0)
> return -1;
>
> return 0;
> @@ -853,7 +848,7 @@ virXMLRewriteFile(int fd, void *opaque)
> return -1;
> }
>
> - if (safewrite(fd, data->xml, strlen(data->xml)) < 0)
> + if (safewrite_str(fd, data->xml) < 0)
> return -1;
>
> return 0;
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 9dd3ba3..62b034c 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -2040,7 +2040,6 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format,
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> char *str = NULL;
> - size_t len;
> const char *lvl = "";
> time_t stTime;
> struct tm stTm;
> @@ -2093,10 +2092,9 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format,
> goto error;
>
> str = virBufferContentAndReset(&buf);
> - len = strlen(str);
>
> /* write log */
> - if (safewrite(ctl->log_fd, str, len) < 0)
> + if (safewrite_str(ctl->log_fd, str) < 0)
> goto error;
>
> VIR_FREE(str);
>
More information about the libvir-list
mailing list