Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen
Am 18.09.2012 20:53, schrieb Jeff Cody:
This is derived from the Supriya Kannery's reopen patches.
This contains the raw-posix driver changes for the bdrv_reopen_*
functions. All changes are staged into a temporary scratch buffer
during the prepare() stage, and copied over to the live structure
during commit(). Upon abort(), all changes are abandoned, and the
live structures are unmodified.
The _prepare() will create an extra fd - either by means of a dup,
if possible, or opening a new fd if not (for instance, access
control changes). Upon _commit(), the original fd is closed and
the new fd is used. Upon _abort(), the duplicate/new fd is closed.
Signed-off-by: Jeff Cody jc...@redhat.com
+static int raw_reopen_prepare(BDRVReopenState *state,
+ BlockReopenQueue *queue, Error **errp)
+{
+BDRVRawState *s;
+BDRVRawReopenState *raw_s;
+int ret = 0;
+
+assert(state != NULL);
+assert(state-bs != NULL);
+
+s = state-bs-opaque;
+
+state-opaque = g_malloc0(sizeof(BDRVRawReopenState));
+raw_s = state-opaque;
+raw_s-use_aio = s-use_aio;
+raw_s-aio_ctx = s-aio_ctx;
You can immediately set s-aio_ctx instead of going through
BDRVRawReopenState with it. It seems to be valid to have it present
while use_aio = 0. If it wasn't valid, you'd have to free the context
when reopening without Linux AIO.
+
+raw_parse_flags(state-flags, raw_s-open_flags);
+raw_set_aio(raw_s-aio_ctx, raw_s-use_aio, state-flags);
At least you're consistently omitting the error check. :-)
+
+raw_s-fd = -1;
+
+int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK;
+#ifdef O_NOATIME
+fcntl_flags |= O_NOATIME;
+#endif
+if ((raw_s-open_flags ~fcntl_flags) == (s-open_flags
~fcntl_flags)) {
+/* dup the original fd */
+/* TODO: use qemu fcntl wrapper */
Hm, still not addressed?
Kevin
Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen
On 09/20/2012 10:10 AM, Kevin Wolf wrote:
Am 18.09.2012 20:53, schrieb Jeff Cody:
This is derived from the Supriya Kannery's reopen patches.
This contains the raw-posix driver changes for the bdrv_reopen_*
functions. All changes are staged into a temporary scratch buffer
during the prepare() stage, and copied over to the live structure
during commit(). Upon abort(), all changes are abandoned, and the
live structures are unmodified.
The _prepare() will create an extra fd - either by means of a dup,
if possible, or opening a new fd if not (for instance, access
control changes). Upon _commit(), the original fd is closed and
the new fd is used. Upon _abort(), the duplicate/new fd is closed.
Signed-off-by: Jeff Cody jc...@redhat.com
+static int raw_reopen_prepare(BDRVReopenState *state,
+ BlockReopenQueue *queue, Error **errp)
+{
+BDRVRawState *s;
+BDRVRawReopenState *raw_s;
+int ret = 0;
+
+assert(state != NULL);
+assert(state-bs != NULL);
+
+s = state-bs-opaque;
+
+state-opaque = g_malloc0(sizeof(BDRVRawReopenState));
+raw_s = state-opaque;
+raw_s-use_aio = s-use_aio;
+raw_s-aio_ctx = s-aio_ctx;
You can immediately set s-aio_ctx instead of going through
BDRVRawReopenState with it. It seems to be valid to have it present
while use_aio = 0. If it wasn't valid, you'd have to free the context
when reopening without Linux AIO.
Good catch, thanks.
+
+raw_parse_flags(state-flags, raw_s-open_flags);
+raw_set_aio(raw_s-aio_ctx, raw_s-use_aio, state-flags);
At least you're consistently omitting the error check. :-)
Thanks, fixed.
I guess I am an optimist at heart :)
+
+raw_s-fd = -1;
+
+int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK;
+#ifdef O_NOATIME
+fcntl_flags |= O_NOATIME;
+#endif
+if ((raw_s-open_flags ~fcntl_flags) == (s-open_flags
~fcntl_flags)) {
+/* dup the original fd */
+/* TODO: use qemu fcntl wrapper */
Hm, still not addressed?
No. I mentioned this in the cover letter for v2. I'd rather see the
qemu fcntl wrapper happen as a separate series, and then come back and
update this, if that is OK. I'm afraid changes to qemu_open or
qemu_dup_flags would delay getting this series in.
Although Eric is right, I do need to ifdef the F_DUPFD_CLOEXEC.
Kevin
Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen
On 09/18/2012 12:53 PM, Jeff Cody wrote:
This is derived from the Supriya Kannery's reopen patches.
This contains the raw-posix driver changes for the bdrv_reopen_*
functions. All changes are staged into a temporary scratch buffer
during the prepare() stage, and copied over to the live structure
during commit(). Upon abort(), all changes are abandoned, and the
live structures are unmodified.
The _prepare() will create an extra fd - either by means of a dup,
if possible, or opening a new fd if not (for instance, access
control changes). Upon _commit(), the original fd is closed and
the new fd is used. Upon _abort(), the duplicate/new fd is closed.
Signed-off-by: Jeff Cody jc...@redhat.com
---
block/raw-posix.c | 99
+++
1 file changed, 99 insertions(+)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6bf5480..edd1eca 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -138,6 +138,15 @@ typedef struct BDRVRawState {
#endif
} BDRVRawState;
+typedef struct BDRVRawReopenState {
+int fd;
+int open_flags;
+#ifdef CONFIG_LINUX_AIO
+int use_aio;
+void *aio_ctx;
+#endif
These members are conditional...
+static int raw_reopen_prepare(BDRVReopenState *state,
+ BlockReopenQueue *queue, Error **errp)
+{
+BDRVRawState *s;
+BDRVRawReopenState *raw_s;
+int ret = 0;
+
+assert(state != NULL);
+assert(state-bs != NULL);
+
+s = state-bs-opaque;
+
+state-opaque = g_malloc0(sizeof(BDRVRawReopenState));
+raw_s = state-opaque;
+raw_s-use_aio = s-use_aio;
+raw_s-aio_ctx = s-aio_ctx;
...but you are unconditionally assigning into them. This will introduce
compile failures on other platforms.
+if ((raw_s-open_flags ~fcntl_flags) == (s-open_flags
~fcntl_flags)) {
+/* dup the original fd */
+/* TODO: use qemu fcntl wrapper */
+raw_s-fd = fcntl(s-fd, F_DUPFD_CLOEXEC, 0);
F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address
your TODO.
+
+/* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */
s/fctnl/fcntl/
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+BDRVRawReopenState *raw_s = state-opaque;
+BDRVRawState *s = state-bs-opaque;
+
+s-open_flags = raw_s-open_flags;
+
+qemu_close(s-fd);
+s-fd = raw_s-fd;
+s-use_aio = raw_s-use_aio;
+s-aio_ctx = raw_s-aio_ctx;
Again, more unconditional use of conditional members.
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen
On 09/18/2012 05:20 PM, Eric Blake wrote:
On 09/18/2012 12:53 PM, Jeff Cody wrote:
This is derived from the Supriya Kannery's reopen patches.
This contains the raw-posix driver changes for the bdrv_reopen_*
functions. All changes are staged into a temporary scratch buffer
during the prepare() stage, and copied over to the live structure
during commit(). Upon abort(), all changes are abandoned, and the
live structures are unmodified.
The _prepare() will create an extra fd - either by means of a dup,
if possible, or opening a new fd if not (for instance, access
control changes). Upon _commit(), the original fd is closed and
the new fd is used. Upon _abort(), the duplicate/new fd is closed.
Signed-off-by: Jeff Cody jc...@redhat.com
---
block/raw-posix.c | 99
+++
1 file changed, 99 insertions(+)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6bf5480..edd1eca 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -138,6 +138,15 @@ typedef struct BDRVRawState {
#endif
} BDRVRawState;
+typedef struct BDRVRawReopenState {
+int fd;
+int open_flags;
+#ifdef CONFIG_LINUX_AIO
+int use_aio;
+void *aio_ctx;
+#endif
These members are conditional...
+static int raw_reopen_prepare(BDRVReopenState *state,
+ BlockReopenQueue *queue, Error **errp)
+{
+BDRVRawState *s;
+BDRVRawReopenState *raw_s;
+int ret = 0;
+
+assert(state != NULL);
+assert(state-bs != NULL);
+
+s = state-bs-opaque;
+
+state-opaque = g_malloc0(sizeof(BDRVRawReopenState));
+raw_s = state-opaque;
+raw_s-use_aio = s-use_aio;
+raw_s-aio_ctx = s-aio_ctx;
...but you are unconditionally assigning into them. This will introduce
compile failures on other platforms.
Argh. Thanks.
+if ((raw_s-open_flags ~fcntl_flags) == (s-open_flags
~fcntl_flags)) {
+/* dup the original fd */
+/* TODO: use qemu fcntl wrapper */
+raw_s-fd = fcntl(s-fd, F_DUPFD_CLOEXEC, 0);
F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address
your TODO.
+
+/* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */
s/fctnl/fcntl/
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+BDRVRawReopenState *raw_s = state-opaque;
+BDRVRawState *s = state-bs-opaque;
+
+s-open_flags = raw_s-open_flags;
+
+qemu_close(s-fd);
+s-fd = raw_s-fd;
+s-use_aio = raw_s-use_aio;
+s-aio_ctx = raw_s-aio_ctx;
Again, more unconditional use of conditional members.
