Re: [Libvir] [PATCH] Rewrite openvzSetUUID.

2008-02-21 Thread Jim Meyering 
Jim Meyering [EMAIL PROTECTED] wrote:
 So here's that same patch without the useless fseek:

 diff --git a/src/openvz_conf.c b/src/openvz_conf.c
 index a274223..2e7b153 100644

I've committed that change:

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

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[Libvir] [PATCH] Rewrite openvzSetUUID.

2008-02-20 Thread Jim Meyering 
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 [EMAIL PROTECTED]
---
 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

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [Libvir] [PATCH] Rewrite openvzSetUUID.

2008-02-20 Thread Jim Paris 
Jim Meyering wrote:
 + /* 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;

I don't think you want to fprintf() if the fseek() fails?

-jim

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [Libvir] [PATCH] Rewrite openvzSetUUID.

2008-02-20 Thread Jim Meyering 
Jim Paris [EMAIL PROTECTED] wrote:

 Jim Meyering wrote:
 +/* 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;

 I don't think you want to fprintf() if the fseek() fails?

Good catch.  Thanks.
I've fixed it this way.

[Hmm.. normally I would have saved and restored errno around the unchecked
fclose call, but I see that the sole caller of openvzSetUUID always
ignores this function's return value.  Sigh... ]

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

 conf_dir = openvzLocateConfDir();
 if (conf_dir == NULL)
@@ -688,23 +687,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)
+if (openvzGetVPSUUID(vpsid, uuidstr))
 return -1;

-ret = openvzGetVPSUUID(vpsid, uuidstr);
-if(ret == -1)
-return -1;
+if (uuidstr[0] == 0) {
+   FILE *fp = fopen(conf_file, a);
+   if (fp == NULL)
+ return -1;

-if(uuidstr[0] == 0) {
 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 fseek, fprintf or fclose fails,
+  and be careful always to close the stream.  */
+   if (fseek(fp, 0, SEEK_END)  0) {
+   fclose(fp);
+   return -1;
+   }
+
+   if ((fprintf(fp, \n#UUID: %s\n, uuidstr)  0)
+   + (fclose(fp) == EOF))
+   return -1;
 }

 return 0;
--
1.5.4.2.134.g82883

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [Libvir] [PATCH] Rewrite openvzSetUUID.

2008-02-20 Thread Jim Meyering 
I wrote:
...
 diff --git a/src/openvz_conf.c b/src/openvz_conf.c
...
 + FILE *fp = fopen(conf_file, a);
 + if (fp == NULL)
 +   return -1;
...
 + /* Record failure if fseek, fprintf or fclose fails,
 +and be careful always to close the stream.  */
 + if (fseek(fp, 0, SEEK_END)  0) {
 + fclose(fp);
 + return -1;
 + }

My thinking cap was not screwed on well enough :-)
The whole point of my using fopen's a (append) flag
was so that I wouldn't have to seek at all.
So here's that same patch without the useless fseek:

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

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

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

-ret = openvzGetVPSUUID(vpsid, uuidstr);
-if(ret == -1)
-return -1;
+if (uuidstr[0] == 0) {
+   FILE *fp = fopen(conf_file, a); /* append */
+   if (fp == NULL)
+ return -1;

-if(uuidstr[0] == 0) {
 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 fprintf or fclose fails,
+  and be careful always to close the stream.  */
+   if ((fprintf(fp, \n#UUID: %s\n, uuidstr)  0)
+   + (fclose(fp) == EOF))
+   return -1;
 }

 return 0;
--
1.5.4.2.157.g733a68

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list