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

[Libvir] [PATCH] Rewrite openvzSetUUID.



There were several unchecked syscalls in this function, along with the
at-least-theoretical risk of a file descriptor leak, so I rewrote this
function to avoid all that, using a stream rather than a bare file
descriptor.

	Subject: [PATCH] Rewrite openvzSetUUID.
	* src/openvz_conf.c (openvzSetUUID): Rewrite to avoid unchecked
	lseek, write, and close as well as a potential file descriptor leak.

Regarding style, the if (expr1 + expr2 + expr3) below might look
odd, but it's better than "if (expr1 || expr2 || expr3)", which
loses if expr1 or expr2 is true, since it short-circuits and
skips expr3, which is the required fclose call.

I could have used "|", but that seemed worse,
and I didn't like the duplication in
  if (expr1)
    ret = -1;
  if (expr2)
    ret = -1;
  if (expr3)
    ret = -1;

In an early iteration, I even wrote this (which still
has too much duplication but isn't that bad):

  ret = 0;
  ret |= e1;
  ret |= e2;
  ret |= e3;

Anyway, just trying to avoid a drawn-out discussion on style....

Signed-off-by: Jim Meyering <meyering redhat com>
---
 src/openvz_conf.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/src/openvz_conf.c b/src/openvz_conf.c
index a274223..1f45c24 100644
--- a/src/openvz_conf.c
+++ b/src/openvz_conf.c
@@ -680,7 +680,7 @@ openvzSetUUID(int vpsid)
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     unsigned char uuid[VIR_UUID_BUFLEN];
     char *conf_dir;
-    int fd, ret;
+    int ret;

     conf_dir = openvzLocateConfDir();
     if (conf_dir == NULL)
@@ -688,26 +688,27 @@ openvzSetUUID(int vpsid)
     sprintf(conf_file, "%s/%d.conf", conf_dir, vpsid);
     free(conf_dir);

-    fd = open(conf_file, O_RDWR);
-    if(fd == -1)
-        return -1;
-
     ret = openvzGetVPSUUID(vpsid, uuidstr);
-    if(ret == -1)
+    if (ret)
         return -1;

-    if(uuidstr[0] == 0) {
+    if (uuidstr[0] == 0) {
+	FILE *fp = fopen(conf_file, "a");
+	if (fp == NULL)
+	  return -1;
+
         virUUIDGenerate(uuid);
         virUUIDFormat(uuid, uuidstr);

-        lseek(fd, 0, SEEK_END);
-        write(fd, "\n#UUID: ", 8);
-        write(fd, uuidstr, strlen(uuidstr));
-        write(fd, "\n", 1);
-        close(fd);
+	/* Record failure if any of these fails,
+	   and be careful always to close the stream.  */
+	if ((fseek(fp, 0, SEEK_END) < 0)
+	    + (fprintf(fp, "\n#UUID: %s\n", uuidstr) < 0);
+	    + (fclose(fp) == EOF))
+	    ret = -1;
     }

-    return 0;
+    return ret;
 }

 /*
--
1.5.4.2.134.g82883


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