"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote:
>> "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
>> > This final patch switches over all places which do fork()/exec() to use the
>> > new enhanced virExec() code. A fair amount of code is deleted, but that's
>> > not the whole story - the new impl is more robust that most of the existing
>> > code we're deleting here.
>>
>> Nice clean-up!
>
> I believe I've addressed all the comments you had - its quite a significant
> change from the previous patch. I was in fact able to simplify the bridge.c
> file even more than I did before.
You did, indeed. Even nicer ;-)
...
> diff -r c3d345142e1b src/qemu_conf.c
> --- a/src/qemu_conf.c Wed Aug 27 12:45:11 2008 +0100
> +++ b/src/qemu_conf.c Wed Aug 27 14:09:26 2008 +0100
> @@ -394,124 +394,100 @@
> }
>
>
> -int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) {
> +int qemudExtractVersionInfo(const char *qemu,
> + unsigned int *retversion,
> + unsigned int *retflags) {
> + const char *const qemuarg[] = { qemu, "-help", NULL };
> + const char *const qemuenv[] = { "LC_ALL=C", NULL };
> pid_t child;
> - int newstdout[2];
> + int newstdout = -1;
> + char help[8192]; /* Ought to be enough to hold QEMU help screen */
> + int got = 0, ret = -1, status;
> + unsigned int major, minor, micro;
> + unsigned int version;
> + unsigned int flags = 0;
>
> - if (flags)
> - *flags = 0;
> - if (version)
> - *version = 0;
> + if (retflags)
> + *retflags = 0;
> + if (retversion)
> + *retversion = 0;
>
> - if (pipe(newstdout) < 0) {
> + if (virExec(NULL, qemuarg, qemuenv, NULL,
> + &child, -1, &newstdout, NULL, VIR_EXEC_NONE) < 0)
> return -1;
> +
> +
> + while (got < (sizeof(help)-1)) {
> + int len;
> + if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0)
> + goto cleanup2;
> + if (!len)
> + break;
> + got += len;
> + }
> + help[got] = '\0';
I was going to ask why you are using saferead, then realized that *I*
suggested the s/read+EINTR/saferead/ change. Now, while re-reviewing this,
I wondered if we could get rid of the 8KB stack buffer and encapsulate
the above loop -- at the expense of allocating the memory instead -- by
using e.g., virFileReadAll. But virFileReadAll operates on a file name,
not a file descriptor. So I wrote/factored a couple of wrappers, and now,
with the following patch, you can use this:
char *help = NULL;
enum { MAX_HELP_OUTPUT_SIZE = 8192 };
int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help);
if (len < 0)
goto ...
...
Then add this somewhere after done with "help":
VIR_FREE(help);
Go ahead and commit your changes.
After you do that, I'll rebase this and propose a complete patch.
>From 620584c1effef8882687c0560dcfaec5a4614c25 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@xxxxxxxxxx>
Date: Thu, 28 Aug 2008 15:54:03 +0200
Subject: [PATCH] util.c: add a file-descriptor-based wrapper for fread_file_lim
* src/util.c (virFileReadLimFP): New function.
(__virFileReadLimFD): New function.
* src/util.h (__virFileReadLimFD): Declare.
(virFileReadLimFD): Define.
(virFileReadAll): Rewrite to use virFileReadLimFP.
---
src/util.c | 71 +++++++++++++++++++++++++++++++++++++++--------------------
src/util.h | 7 +++--
2 files changed, 51 insertions(+), 27 deletions(-)
diff --git a/src/util.c b/src/util.c
index a81af07..fd30778 100644
--- a/src/util.c
+++ b/src/util.c
@@ -510,40 +510,63 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length)
return NULL;
}
-int __virFileReadAll(const char *path, int maxlen, char **buf)
+/* A wrapper around fread_file_lim that maps a failure due to
+ exceeding the maximum size limitation to EOVERFLOW. */
+static int virFileReadLimFP(FILE *fp, int maxlen, char **buf)
{
- FILE *fh;
- int ret = -1;
size_t len;
- char *s;
+ char *s = fread_file_lim (fp, maxlen+1, &len);
+ if (s == NULL)
+ return -1;
+ if (len > maxlen || (int)len != len) {
+ VIR_FREE(s);
+ /* There was at least one byte more than MAXLEN.
+ Set errno accordingly. */
+ errno = EOVERFLOW;
+ return -1;
+ }
+ *buf = s;
+ return len;
+}
+
+/* Like virFileReadLimFP, but use a file descriptor rather than a FILE*. */
+int __virFileReadLimFD(int fd_arg, int maxlen, char **buf)
+{
+ int fd = dup (fd_arg);
+ if (0 <= fd) {
+ FILE *fp = fdopen (fd, "r");
+ if (fp) {
+ int len = virFileReadLimFP (fp, maxlen, buf);
+ int saved_errno = errno;
+ fclose (fp);
+ errno = saved_errno;
+ return len;
+ } else {
+ int saved_errno = errno;
+ close (fd);
+ errno = saved_errno;
+ }
+ }
+ return -1;
+}
- if (!(fh = fopen(path, "r"))) {
+int __virFileReadAll(const char *path, int maxlen, char **buf)
+{
+ FILE *fh = fopen(path, "r");
+ if (fh == NULL) {
virLog("Failed to open file '%s': %s\n",
path, strerror(errno));
- goto error;
+ return -1;
}
- s = fread_file_lim(fh, maxlen+1, &len);
- if (s == NULL) {
+ int len = virFileReadLimFP (fh, maxlen, buf);
+ fclose(fh);
+ if (len < 0) {
virLog("Failed to read '%s': %s\n", path, strerror (errno));
- goto error;
- }
-
- if (len > maxlen || (int)len != len) {
- VIR_FREE(s);
- virLog("File '%s' is too large %d, max %d\n",
- path, (int)len, maxlen);
- goto error;
+ return -1;
}
- *buf = s;
- ret = len;
-
- error:
- if (fh)
- fclose(fh);
-
- return ret;
+ return len;
}
int virFileMatchesNameSuffix(const char *file,
diff --git a/src/util.h b/src/util.h
index 5151582..f2da006 100644
--- a/src/util.h
+++ b/src/util.h
@@ -45,9 +45,10 @@ int virExec(virConnectPtr conn,
int flags);
int virRun(virConnectPtr conn, const char *const*argv, int *status);
-int __virFileReadAll(const char *path,
- int maxlen,
- char **buf);
+int __virFileReadLimFD(int fd, int maxlen, char **buf);
+#define virFileReadLimFD(fd,m,b) __virFileReadLimFD((fd),(m),(b))
+
+int __virFileReadAll(const char *path, int maxlen, char **buf);
#define virFileReadAll(p,m,b) __virFileReadAll((p),(m),(b))
int virFileMatchesNameSuffix(const char *file,
--
1.6.0.1.126.ga118
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list