Re: [PATCH 3/4] virpidfile: replace fopen/fwrite/fscanf with more portable version

[Eric Blake <eblake@xxxxxxxxxx>]

On 02/01/2012 05:28 PM, Marc-André Lureau wrote:
> Replace calls to fwrite() and fscanf() with more portable-friendly
> version, such as snprintf() and virStrToLong().

I'm tweaking this to mention why snprintf is more portable - because
we're using gnulib for snprintf but not for fprintf.

> ---
>  src/util/virpidfile.c |   42 +++++++++++++++++++++++++-----------------
>  1 files changed, 25 insertions(+), 17 deletions(-)

> @@ -63,21 +63,18 @@ int virPidFileWritePath(const char *pidfile,
>          goto cleanup;
>      }
>  
> -    if (!(file = VIR_FDOPEN(fd, "w"))) {
> -        rc = -errno;
> -        VIR_FORCE_CLOSE(fd);
> -        goto cleanup;
> -    }
> +    snprintf(pidstr, sizeof(pidstr), "%" PID_FORMAT, pid);
>  
> -    if (fprintf(file, "%" PID_FORMAT, pid) < 0) {

I'm using %lld, (long long) pid here.

> @@ -117,30 +114,41 @@ cleanup:
>  int virPidFileReadPath(const char *path,
>                         pid_t *pid)
>  {
> -    FILE *file;
> +    int fd;
>      int rc;
> +    ssize_t bytes;
> +    char pidstr[INT_BUFSIZE_BOUND(*pid)];

It's easier to scan into a long long, then ensure that conversion to
pid_t doesn't truncate.

> -    if (VIR_FCLOSE(file) < 0) {
> -        rc = -errno;
> +#if SIZEOF_PID_T == 8
> +    if (virStrToLong_ll(pidstr, NULL, 10, pid) < 0) {
> +#elif SIZEOF_PID_T == 4
> +    if (virStrToLong_i(pidstr, NULL, 10, pid) < 0) {
> +#endif

That is, I'm not a fan of in-method #ifdefs, if they can be avoided by
other means.

ACK; I'm pushing it slightly amended as:

diff --git c/src/util/virpidfile.c w/src/util/virpidfile.c
index 1fd6318..34d1250 100644
--- c/src/util/virpidfile.c
+++ w/src/util/virpidfile.c
@@ -1,7 +1,7 @@
 /*
  * virpidfile.c: manipulation of pidfiles
  *
- * Copyright (C) 2010-2011 Red Hat, Inc.
+ * Copyright (C) 2010-2012 Red Hat, Inc.
  * Copyright (C) 2006, 2007 Binary Karma
  * Copyright (C) 2006 Shuveb Hussain
  *
@@ -54,7 +54,7 @@ int virPidFileWritePath(const char *pidfile,
 {
     int rc;
     int fd;
-    FILE *file = NULL;
+    char pidstr[INT_BUFSIZE_BOUND(pid)];

     if ((fd = open(pidfile,
                    O_WRONLY | O_CREAT | O_TRUNC,
@@ -63,21 +63,18 @@ int virPidFileWritePath(const char *pidfile,
         goto cleanup;
     }

-    if (!(file = VIR_FDOPEN(fd, "w"))) {
-        rc = -errno;
-        VIR_FORCE_CLOSE(fd);
-        goto cleanup;
-    }
+    snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);

-    if (fprintf(file, "%d", pid) < 0) {
+    if (safewrite(fd, pidstr, strlen(pidstr)) < 0) {
         rc = -errno;
+        VIR_FORCE_CLOSE(fd);
         goto cleanup;
     }

     rc = 0;

 cleanup:
-    if (VIR_FCLOSE(file) < 0)
+    if (VIR_CLOSE(fd) < 0)
         rc = -errno;

     return rc;
@@ -117,30 +114,40 @@ cleanup:
 int virPidFileReadPath(const char *path,
                        pid_t *pid)
 {
-    FILE *file;
+    int fd;
     int rc;
+    ssize_t bytes;
+    long long pid_value = 0;
+    char pidstr[INT_BUFSIZE_BOUND(pid_value)];

     *pid = 0;

-    if (!(file = fopen(path, "r"))) {
+    if ((fd = open(path, O_RDONLY)) < 0) {
         rc = -errno;
         goto cleanup;
     }

-    if (fscanf(file, "%d", pid) != 1) {
-        rc = -EINVAL;
-        VIR_FORCE_FCLOSE(file);
+    bytes = saferead(fd, pidstr, sizeof(pidstr));
+    if (bytes < 0) {
+        rc = -errno;
+        VIR_FORCE_CLOSE(fd);
         goto cleanup;
     }
+    pidstr[bytes] = '\0';

-    if (VIR_FCLOSE(file) < 0) {
-        rc = -errno;
+    if (virStrToLong_ll(pidstr, NULL, 10, &pid_value) < 0 ||
+        (pid_t) pid_value != pid_value) {
+        rc = -1;
         goto cleanup;
     }

+    *pid = pid_value;
     rc = 0;

- cleanup:
+cleanup:
+    if (VIR_CLOSE(fd) < 0)
+        rc = -errno;
+
     return rc;
 }

@@ -367,7 +374,7 @@ int virPidFileAcquirePath(const char *path,
         /* Someone else must be racing with us, so try agin */
     }

-    snprintf(pidstr, sizeof(pidstr), "%u", (unsigned int)pid);
+    snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);

     if (safewrite(fd, pidstr, strlen(pidstr)) < 0) {
         virReportSystemError(errno,


-- 
Eric Blake   eblake@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

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