[PATCH 1/3] daemon: pwrite/pread: Don't double close on error path.
From: "Richard W.M. Jones" <rjones@redhat.com> In Linux, close (fd) closes the file descriptor even if it returns an error. --- daemon/file.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/daemon/file.c b/daemon/file.c index e0f8794..91746e0 100644 --- a/daemon/file.c +++ b/daemon/file.c @@ -483,7 +483,6 @@ pread_fd (int fd, int count, int64_t offset, size_t *size_r, if (close (fd) == -1) { reply_with_perror ("close: %s", display_path); - close (fd); free (buf); return NULL; } @@ -539,7 +538,6 @@ pwrite_fd (int fd, const char *content, size_t size, int64_t offset, if (close (fd) == -1) { reply_with_perror ("close: %s", display_path); - close (fd); return -1; } -- 1.7.6
From: "Richard W.M. Jones" <rjones@redhat.com> This allows us to use 'fuser' and other ps tools. --- appliance/packagelist.in | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/appliance/packagelist.in b/appliance/packagelist.in index 550ba8b..5735a5a 100644 --- a/appliance/packagelist.in +++ b/appliance/packagelist.in @@ -119,6 +119,7 @@ ocfs2-tools */ parted procps +psmisc scrub strace tar -- 1.7.6
From: "Richard W.M. Jones" <rjones@redhat.com> 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. --- daemon/file.c | 17 ++++++++++++++--- daemon/parted.c | 3 ++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/daemon/file.c b/daemon/file.c index 91746e0..3e42c1e 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) + const char *display_path, int settle) { ssize_t r; @@ -541,6 +541,17 @@ 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. + * + * 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; } @@ -563,7 +574,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); + return pwrite_fd (fd, content, size, offset, path, 0); } int @@ -581,7 +592,7 @@ do_pwrite_device (const char *device, const char *content, size_t size, return -1; } - return pwrite_fd (fd, content, size, offset, device); + return pwrite_fd (fd, content, size, offset, device, 1); } /* This runs the 'file' command. */ diff --git a/daemon/parted.c b/daemon/parted.c index 16f0843..64a7d1d 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -34,7 +34,8 @@ * 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 + * probably because udev monitors all 'close' on block devices + * and runs 'blkid' which opens and examines the device. We attempt * to detect and recover from this error if we can. */ static int -- 1.7.6
Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
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. --- daemon/file.c | 17 ++++++++++++++--- daemon/parted.c | 3 ++- 2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/daemon/file.c b/daemon/file.c index 91746e0..3e42c1e 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) + const char *display_path, int settle) { ssize_t r;
@@ -541,6 +541,17 @@ 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. + * + * 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.
This series looks fine. Only question is what about non-block devices? How about honoring "settle" only when fd is open on a block device? if (fstat (fd, &st) == 0 && S_ISBLK (st.st_mode) && settle) udev_settle ();
+ if (settle) + udev_settle (); + return r; }
@@ -563,7 +574,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); + return pwrite_fd (fd, content, size, offset, path, 0); }
int @@ -581,7 +592,7 @@ do_pwrite_device (const char *device, const char *content, size_t size, return -1; }
- return pwrite_fd (fd, content, size, offset, device); + return pwrite_fd (fd, content, size, offset, device, 1); } ...
On Thu, Jan 19, 2012 at 03:36:49PM +0100, Jim Meyering wrote:
Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
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. --- daemon/file.c | 17 ++++++++++++++--- daemon/parted.c | 3 ++- 2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/daemon/file.c b/daemon/file.c index 91746e0..3e42c1e 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) + const char *display_path, int settle) { ssize_t r;
@@ -541,6 +541,17 @@ 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. + * + * 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.
This series looks fine. Only question is what about non-block devices?
How about honoring "settle" only when fd is open on a block device?
if (fstat (fd, &st) == 0 && S_ISBLK (st.st_mode) && settle) udev_settle ();
That's done already because the (generated) surrounding code ensures that do_pwrite is only called with a path in the guest filesystem, whereas do_pwrite_device is only called with a device name. Hence the 'settle' flag initialized here:
+ if (settle) + udev_settle (); + return r; }
@@ -563,7 +574,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); + return pwrite_fd (fd, content, size, offset, path, 0); }
int @@ -581,7 +592,7 @@ do_pwrite_device (const char *device, const char *content, size_t size, return -1; }
- return pwrite_fd (fd, content, size, offset, device); + return pwrite_fd (fd, content, size, offset, device, 1); } ...
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
participants (2)
-
Jim Meyering -
Richard W.M. Jones