"Richard W.M. Jones" <rjones@xxxxxxxxxx> wrote:
> On Tue, Apr 08, 2008 at 05:00:03PM +0200, Jim Meyering wrote:
>> This fix addresses a problem exposed in an ovirt script whereby
>> trying to use bash process substitution, e.g., in
>> virsh define <(command to generate xml)
>> would fail.
>>
>> Oops. Just noticed that the indentation in the added function
>> (gnulib style) is not consistent with the rest of the file.
>> I'll adjust that before committing, of course.
>>
>> Don't fail to read a file because it's non-seekable (e.g., a pipe).
>> * src/util.c (fread_file_lim): New function.
>> (__virFileReadAll): Use fread_file_lim, rather than requiring
>> that stat.st_size provide a usable file size.
>> * tests/read-non-seekable: New test, for the above.
>> * tests/Makefile.am (test_scripts): Add read-non-seekable.
>> * tests/test-lib.sh (mkfifo_or_skip_): New helper function.
>
> This fix looks good. In fact I'd go further and remove the test for
> S_ISDIR(st.st_mode) and the stat buffer altogether.
Thanks for the quick feedback.
Yeah, that's probably cleaner.
And if someone does e.g. virsh define DIR_NAME
on a system that lets you read directories, they
won't be too surprised that the contents of the
directory is invalid XML.
Here's the updated patch:
Don't fail to read a file because it's non-seekable (e.g., a pipe).
* src/util.c (fread_file_lim): New function.
(__virFileReadAll): Use fread_file_lim, rather than requiring
that stat.st_size provide a usable file size.
* tests/read-non-seekable: New test, for the above.
* tests/Makefile.am (test_scripts): Add read-non-seekable.
* tests/test-lib.sh (mkfifo_or_skip_): New helper function.
Signed-off-by: Jim Meyering <meyering@xxxxxxxxxx>
---
src/util.c | 99 +++++++++++++++++++++++++++++++---------------
tests/Makefile.am | 1 +
tests/read-non-seekable | 47 ++++++++++++++++++++++
tests/test-lib.sh | 12 ++++++
4 files changed, 127 insertions(+), 32 deletions(-)
create mode 100755 tests/read-non-seekable
diff --git a/src/util.c b/src/util.c
index 0667780..801f615 100644
--- a/src/util.c
+++ b/src/util.c
@@ -49,6 +49,10 @@
#include "util-lib.c"
+#ifndef MIN
+# define MIN(a, b) ((a) < (b) ? (a) : (b))
+#endif
+
#define MAX_ERROR_LEN 1024
#define TOLOWER(Ch) (isupper (Ch) ? tolower (Ch) : (Ch))
@@ -283,14 +287,64 @@ virExecNonBlock(virConnectPtr conn,
#endif /* __MINGW32__ */
+/* Like gnulib's fread_file, but read no more than the specified maximum
+ number of bytes. If the length of the input is <= max_len, and
+ upon error while reading that data, it works just like fread_file. */
+static char *
+fread_file_lim (FILE *stream, size_t max_len, size_t *length)
+{
+ char *buf = NULL;
+ size_t alloc = 0;
+ size_t size = 0;
+ int save_errno;
+
+ for (;;) {
+ size_t count;
+ size_t requested;
+
+ if (size + BUFSIZ + 1 > alloc) {
+ char *new_buf;
+
+ alloc += alloc / 2;
+ if (alloc < size + BUFSIZ + 1)
+ alloc = size + BUFSIZ + 1;
+
+ new_buf = realloc (buf, alloc);
+ if (!new_buf) {
+ save_errno = errno;
+ break;
+ }
+
+ buf = new_buf;
+ }
+
+ /* Ensure that (size + requested <= max_len); */
+ requested = MIN (size < max_len ? max_len - size : 0,
+ alloc - size - 1);
+ count = fread (buf + size, 1, requested, stream);
+ size += count;
+
+ if (count != requested || requested == 0) {
+ save_errno = errno;
+ if (ferror (stream))
+ break;
+ buf[size] = '\0';
+ *length = size;
+ return buf;
+ }
+ }
+
+ free (buf);
+ errno = save_errno;
+ return NULL;
+}
-int __virFileReadAll(const char *path,
- int maxlen,
- char **buf)
+int __virFileReadAll(const char *path, int maxlen, char **buf)
{
FILE *fh;
- struct stat st;
int ret = -1;
+ size_t len;
+ char *s;
if (!(fh = fopen(path, "r"))) {
virLog("Failed to open file '%s': %s",
@@ -298,39 +352,21 @@ int __virFileReadAll(const char *path,
goto error;
}
- if (fstat(fileno(fh), &st) < 0) {
- virLog("Failed to stat file '%s': %s",
- path, strerror(errno));
+ s = fread_file_lim(fh, maxlen+1, &len);
+ if (s == NULL) {
+ virLog("Failed to read '%s': %s", path, strerror (errno));
goto error;
}
- if (S_ISDIR(st.st_mode)) {
- virLog("Ignoring directory '%s'", path);
+ if (len > maxlen || (int)len != len) {
+ free(s);
+ virLog("File '%s' is too large %d, max %d",
+ path, (int)len, maxlen);
goto error;
}
- if (st.st_size > maxlen) {
- virLog("File '%s' is too large %d, max %d", path, (int)st.st_size, maxlen);
- goto error;
- }
-
- *buf = malloc(st.st_size + 1);
- if (*buf == NULL) {
- virLog("Failed to allocate data");
- goto error;
- }
-
- if ((ret = fread(*buf, st.st_size, 1, fh)) != 1) {
- free(*buf);
- *buf = NULL;
- virLog("Failed to read config file '%s': %s",
- path, strerror(errno));
- goto error;
- }
-
- (*buf)[st.st_size] = '\0';
-
- ret = st.st_size;
+ *buf = s;
+ ret = len;
error:
if (fh)
@@ -739,6 +775,5 @@ virParseMacAddr(const char* str, unsigned char *addr)
* indent-tabs-mode: nil
* c-indent-level: 4
* c-basic-offset: 4
- * tab-width: 4
* End:
*/
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 901e88a..ca12b84 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -46,6 +46,7 @@ noinst_PROGRAMS = xmlrpctest xml2sexprtest sexpr2xmltest virshtest conftest \
test_scripts = \
daemon-conf \
int-overflow \
+ read-non-seekable \
vcpupin
EXTRA_DIST += $(test_scripts)
diff --git a/tests/read-non-seekable b/tests/read-non-seekable
new file mode 100755
index 0000000..9bb4a21
--- /dev/null
+++ b/tests/read-non-seekable
@@ -0,0 +1,47 @@
+#!/bin/sh
+# ensure that certain file-reading commands can handle non-seekable files
+
+# Copyright (C) 2008 Free Software Foundation, Inc.
+
+# 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, either version 3 of the License, or
+# (at your option) any later version.
+
+# 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/>.
+
+if test "$VERBOSE" = yes; then
+ set -x
+ virsh --version
+fi
+
+. $srcdir/test-lib.sh
+
+fail=0
+
+cat <<\EOF > dom
+<domain type='test' id='2'>
+ <name>t2</name>
+ <uuid>004b96e1-2d78-c30f-5aa5-000000000000</uuid>
+ <memory>8388608</memory>
+ <vcpu>2</vcpu>
+ <on_reboot>restart</on_reboot>
+ <on_poweroff>destroy</on_poweroff>
+ <on_crash>restart</on_crash>
+</domain>
+EOF
+
+virsh -c test:///default define dom > /dev/null || fail=1
+
+mkfifo_or_skip_ fifo
+cat dom > fifo &
+
+virsh -c test:///default define fifo > /dev/null || fail=1
+
+(exit $fail); exit $fail
diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index cdbea5d..a007109 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -120,6 +120,18 @@ skip_if_root_() { uid_is_privileged_ && skip_test_ "must be run as non-root"; }
error_() { echo "$0: $@" 1>&2; (exit 1); exit 1; }
framework_failure() { error_ 'failure in testing framework'; }
+mkfifo_or_skip_()
+{
+ test $# = 1 || framework_failure
+ if ! mkfifo "$1"; then
+ # Make an exception of this case -- usually we interpret framework-creation
+ # failure as a test failure. However, in this case, when running on a SunOS
+ # system using a disk NFS mounted from OpenBSD, the above fails like this:
+ # mkfifo: cannot make fifo `fifo-10558': Not owner
+ skip_test_ 'NOTICE: unable to create test prerequisites'
+ fi
+}
+
test_dir_=$(pwd)
this_test_() { echo "./$0" | sed 's,.*/,,'; }
--
1.5.5.rc2.7.g144a
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list