Libguestfs - Feb 2012 - [PATCH 1/2] Revert "daemon: Run udev_settle after pwrite-device finishes."

From: "Richard W.M. Jones" <rjones at redhat.com>

This reverts commit a9c8123c72db47bcab8dd738e8d5256a9ae87f11.
---
 daemon/file.c   |   18 +++---------------
 daemon/parted.c |    3 +--
 2 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/daemon/file.c b/daemon/file.c
index 057e15d..91746e0 100644
--- a/daemon/file.c
+++ b/daemon/file.c
@@ -525,7 +525,7 @@ do_pread_device (const char *device, int count, int64_t
offset, size_t *size_r)
 
 static int
 pwrite_fd (int fd, const char *content, size_t size, int64_t offset,
-           const char *display_path, int settle)
+           const char *display_path)
 {
   ssize_t r;
 
@@ -541,18 +541,6 @@ pwrite_fd (int fd, const char *content, size_t size,
int64_t offset,
     return -1;
   }
 
-  /* When you call close on any block device, udev kicks off a rule
-   * which runs blkid to reexamine the device.  We need to wait for
-   * this rule to finish running since it holds the device open and
-   * can cause other operations to fail, notably BLKRRPART.  'settle'
-   * flag is only set on block devices.
-   *
-   * XXX We should be smarter about when we do this or should get rid
-   * of the udev rules since we don't use blkid in cached mode.
-   */
-  if (settle)
-    udev_settle ();
-
   return r;
 }
 
@@ -575,7 +563,7 @@ do_pwrite (const char *path, const char *content, size_t
size, int64_t offset)
     return -1;
   }
 
-  return pwrite_fd (fd, content, size, offset, path, 0);
+  return pwrite_fd (fd, content, size, offset, path);
 }
 
 int
@@ -593,7 +581,7 @@ do_pwrite_device (const char *device, const char *content,
size_t size,
     return -1;
   }
 
-  return pwrite_fd (fd, content, size, offset, device, 1);
+  return pwrite_fd (fd, content, size, offset, device);
 }
 
 /* This runs the 'file' command. */
diff --git a/daemon/parted.c b/daemon/parted.c
index 64a7d1d..16f0843 100644
--- a/daemon/parted.c
+++ b/daemon/parted.c
@@ -34,8 +34,7 @@
  * COMMAND_FLAG_FOLD_STDOUT_ON_STDERR flag.
  *
  * parted occasionally fails to do ioctl(BLKRRPART) on the device,
- * probably because udev monitors all 'close' on block devices
- * and runs 'blkid' which opens and examines the device.  We attempt
+ * apparently because of some internal race in the code.  We attempt
  * to detect and recover from this error if we can.
  */
 static int
-- 
1.7.6

From: "Richard W.M. Jones" <rjones at redhat.com>

See comments in the code for details.

This is an alternate fix to
commit a9c8123c72db47bcab8dd738e8d5256a9ae87f11.
---
 daemon/blockdev.c |   14 +++++
 daemon/parted.c   |  153 +++++++++++++++++++++++++++++++++++------------------
 2 files changed, 115 insertions(+), 52 deletions(-)

diff --git a/daemon/blockdev.c b/daemon/blockdev.c
index d056abe..a7fd2cb 100644
--- a/daemon/blockdev.c
+++ b/daemon/blockdev.c
@@ -46,6 +46,20 @@ call_blockdev (const char *device, const char *switc, int
extraarg, int prints)
   };
   char buf[64];
 
+  /* When you call close on any block device, udev kicks off a rule
+   * which runs blkid to reexamine the device.  We need to wait for
+   * this rule to finish running (from a previous operation) since it
+   * holds the device open and can cause other operations to fail,
+   * notably BLKRRPART.
+   *
+   * This is particularly a problem where we have just written to a
+   * device (eg. zeroing it) and immediately call blockdev --rereadpt.
+   *
+   * Therefore, wait for udev to finish all outstanding events before
+   * performing any blockdev command.
+   */
+  udev_settle ();
+
   if (extraarg > 0) {
     snprintf (buf, sizeof buf, "%d", extraarg);
     argv[2] = buf;
diff --git a/daemon/parted.c b/daemon/parted.c
index 16f0843..2610042 100644
--- a/daemon/parted.c
+++ b/daemon/parted.c
@@ -33,45 +33,14 @@
  * Parted 1.9 sends error messages to stdout, hence use of the
  * COMMAND_FLAG_FOLD_STDOUT_ON_STDERR flag.
  *
- * parted occasionally fails to do ioctl(BLKRRPART) on the device,
- * apparently because of some internal race in the code.  We attempt
- * to detect and recover from this error if we can.
+ * There is a reason why we call udev_settle both before and after
+ * each command.  When you call close on any block device, udev kicks
+ * off a rule which runs blkid to reexamine the device.  We need to
+ * wait for this rule to finish running (from a previous operation)
+ * since it holds the device open.  Since parted also closes the block
+ * device, it can cause udev to run again, hence the call to
+ * udev_settle afterwards.
  */
-static int
-recover_blkrrpart (const char *device, const char *err)
-{
-  int r;
-
-  if (!strstr (err,
-               "Error informing the kernel about modifications to
partition"))
-    return -1;
-
-  r = command (NULL, NULL, "blockdev", "--rereadpt",
device, NULL);
-  if (r == -1)
-    return -1;
-
-  udev_settle ();
-
-  return 0;
-}
-
-#define RUN_PARTED(error,device,...)                                    \
-  do {                                                                  \
-    int r;                                                              \
-    char *err;                                                          \
-                                                                        \
-    r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR,       \
-                  "parted", "-s", "--", (device),
__VA_ARGS__);   \
-    if (r == -1) {                                                      \
-      if (recover_blkrrpart ((device), err) == -1) {                    \
-        reply_with_error ("%s: parted: %s: %s", __func__, (device),
err); \
-        free (err);                                                     \
-        error;                                                          \
-      }                                                                 \
-    }                                                                   \
-                                                                        \
-    free (err);                                                         \
-  } while (0)
 
 static const char *
 check_parttype (const char *parttype)
@@ -101,13 +70,25 @@ check_parttype (const char *parttype)
 int
 do_part_init (const char *device, const char *parttype)
 {
+  int r;
+  char *err;
+
   parttype = check_parttype (parttype);
   if (!parttype) {
     reply_with_error ("unknown partition type: common choices are
\"gpt\" and \"msdos\"");
     return -1;
   }
 
-  RUN_PARTED (return -1, device, "mklabel", parttype, NULL);
+  udev_settle ();
+
+  r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR,
+                "parted", "-s", "--", device,
"mklabel", parttype, NULL);
+  if (r == -1) {
+    reply_with_error ("parted: %s: %s", device, err);
+    free (err);
+    return -1;
+  }
+  free (err);
 
   udev_settle ();
 
@@ -118,6 +99,8 @@ int
 do_part_add (const char *device, const char *prlogex,
              int64_t startsect, int64_t endsect)
 {
+  int r;
+  char *err;
   char startstr[32];
   char endstr[32];
 
@@ -146,12 +129,22 @@ do_part_add (const char *device, const char *prlogex,
   snprintf (startstr, sizeof startstr, "%" PRIi64 "s",
startsect);
   snprintf (endstr, sizeof endstr, "%" PRIi64 "s",
endsect);
 
+  udev_settle ();
+
   /* XXX Bug: If the partition table type (which we don't know in this
    * function) is GPT, then this parted command sets the _partition
    * name_ to prlogex, eg. "primary".  I would essentially describe
    * this as a bug in the parted mkpart command.
    */
-  RUN_PARTED (return -1, device, "mkpart", prlogex, startstr, endstr,
NULL);
+  r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR,
+                "parted", "-s", "--",
+                device, "mkpart", prlogex, startstr, endstr, NULL);
+  if (r == -1) {
+    reply_with_error ("parted: %s: %s", device, err);
+    free (err);
+    return -1;
+  }
+  free (err);
 
   udev_settle ();
 
@@ -161,6 +154,9 @@ do_part_add (const char *device, const char *prlogex,
 int
 do_part_del (const char *device, int partnum)
 {
+  int r;
+  char *err;
+
   if (partnum <= 0) {
     reply_with_error ("partition number must be >= 1");
     return -1;
@@ -169,15 +165,28 @@ do_part_del (const char *device, int partnum)
   char partnum_str[16];
   snprintf (partnum_str, sizeof partnum_str, "%d", partnum);
 
-  RUN_PARTED (return -1, device, "rm", partnum_str, NULL);
+  udev_settle ();
+
+  r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR,
+                "parted", "-s", "--", device,
"rm", partnum_str, NULL);
+  if (r == -1) {
+    reply_with_error ("parted: %s: %s", device, err);
+    free (err);
+    return -1;
+  }
+  free (err);
 
   udev_settle ();
+
   return 0;
 }
 
 int
 do_part_disk (const char *device, const char *parttype)
 {
+  int r;
+  char *err;
+
   parttype = check_parttype (parttype);
   if (!parttype) {
     reply_with_error ("unknown partition type: common choices are
\"gpt\" and \"msdos\"");
@@ -195,12 +204,21 @@ do_part_disk (const char *device, const char *parttype)
   const char *startstr = "128s";
   const char *endstr = "-128s";
 
-  RUN_PARTED (return -1,
-              device,
-              "mklabel", parttype,
-              /* See comment about about the parted mkpart command. */
-              "mkpart", STREQ (parttype, "gpt") ?
"p1" : "primary",
-              startstr, endstr, NULL);
+  udev_settle ();
+
+  r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR,
+                "parted", "-s", "--",
+                device,
+                "mklabel", parttype,
+                /* See comment about about the parted mkpart command. */
+                "mkpart", STREQ (parttype, "gpt") ?
"p1" : "primary",
+                startstr, endstr, NULL);
+  if (r == -1) {
+    reply_with_error ("parted: %s: %s", device, err);
+    free (err);
+    return -1;
+  }
+  free (err);
 
   udev_settle ();
 
@@ -210,6 +228,9 @@ do_part_disk (const char *device, const char *parttype)
 int
 do_part_set_bootable (const char *device, int partnum, int bootable)
 {
+  int r;
+  char *err;
+
   if (partnum <= 0) {
     reply_with_error ("partition number must be >= 1");
     return -1;
@@ -219,8 +240,17 @@ do_part_set_bootable (const char *device, int partnum, int
bootable)
 
   snprintf (partstr, sizeof partstr, "%d", partnum);
 
-  RUN_PARTED (return -1,
-              device, "set", partstr, "boot", bootable ?
"on" : "off", NULL);
+  udev_settle ();
+
+  r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR,
+                "parted", "-s", "--",
+                device, "set", partstr, "boot", bootable ?
"on" : "off", NULL);
+  if (r == -1) {
+    reply_with_error ("parted: %s: %s", device, err);
+    free (err);
+    return -1;
+  }
+  free (err);
 
   udev_settle ();
 
@@ -230,6 +260,9 @@ do_part_set_bootable (const char *device, int partnum, int
bootable)
 int
 do_part_set_name (const char *device, int partnum, const char *name)
 {
+  int r;
+  char *err;
+
   if (partnum <= 0) {
     reply_with_error ("partition number must be >= 1");
     return -1;
@@ -239,7 +272,16 @@ do_part_set_name (const char *device, int partnum, const
char *name)
 
   snprintf (partstr, sizeof partstr, "%d", partnum);
 
-  RUN_PARTED (return -1, device, "name", partstr, name, NULL);
+  udev_settle ();
+
+  r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR,
+                "parted", "-s", "--", device,
"name", partstr, name, NULL);
+  if (r == -1) {
+    reply_with_error ("parted: %s: %s", device, err);
+    free (err);
+    return -1;
+  }
+  free (err);
 
   udev_settle ();
 
@@ -686,6 +728,8 @@ do_part_get_mbr_id (const char *device, int partnum)
   char *out, *err;
   int r;
 
+  udev_settle ();
+
   r = command (&out, &err, "sfdisk", "--print-id",
device, partnum_str, NULL);
   if (r == -1) {
     reply_with_error ("sfdisk --print-id: %s", err);
@@ -693,9 +737,10 @@ do_part_get_mbr_id (const char *device, int partnum)
     free (err);
     return -1;
   }
-
   free (err);
 
+  udev_settle ();
+
   /* It's printed in hex ... */
   int id;
   if (sscanf (out, "%x", &id) != 1) {
@@ -725,6 +770,8 @@ do_part_set_mbr_id (const char *device, int partnum, int
idbyte)
   char *err;
   int r;
 
+  udev_settle ();
+
   r = command (NULL, &err, "sfdisk",
                "--change-id", device, partnum_str, idbyte_str, NULL);
   if (r == -1) {
@@ -732,7 +779,9 @@ do_part_set_mbr_id (const char *device, int partnum, int
idbyte)
     free (err);
     return -1;
   }
-
   free (err);
+
+  udev_settle ();
+
   return 0;
 }
-- 
1.7.6

Ignore this repeated patch ... git send-email confused me and I
thought it hadn't sent the patch out first time!

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/