[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] Don't fail to read a file because it's non-seekable (e.g., a pipe).



"Richard W.M. Jones" <rjones redhat com> 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 redhat com>
---
 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]