* [Qemu-devel] [PATCH V2] disable blkverify external snapshot creation
@ 2013-09-26 14:33 Benoît Canet
2013-09-26 14:33 ` [Qemu-devel] [PATCH V2] block: Add BlockDriver.bdrv_check_ext_snapshot Benoît Canet
0 siblings, 1 reply; 4+ messages in thread
From: Benoît Canet @ 2013-09-26 14:33 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jcody, Benoît Canet, stefanha
Hello,
Here is V2 of the external snapshot disabling patch.
The result is hopefully smaller and don't impact all BlockDriver anymore.
Only the blkverify Driver is modified.
v2:
Use NULL fields to avoid having to fill the new field in every BlockDriver
[Jeff]
Rename the field [Kevin]
Benoît Canet (1):
block: Add BlockDriver.bdrv_check_ext_snapshot.
block.c | 14 ++++++++++++++
block/blkverify.c | 2 ++
blockdev.c | 5 +++++
include/block/block.h | 7 +++++++
include/block/block_int.h | 8 ++++++++
5 files changed, 36 insertions(+)
--
1.8.1.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] [PATCH V2] block: Add BlockDriver.bdrv_check_ext_snapshot.
2013-09-26 14:33 [Qemu-devel] [PATCH V2] disable blkverify external snapshot creation Benoît Canet
@ 2013-09-26 14:33 ` Benoît Canet
2013-09-26 16:48 ` Jeff Cody
0 siblings, 1 reply; 4+ messages in thread
From: Benoît Canet @ 2013-09-26 14:33 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jcody, Benoît Canet, stefanha
This field is used by blkverify to disable external snapshots creation.
I will also be used by block filters like quorum to disable external snapshots
creation.
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 14 ++++++++++++++
block/blkverify.c | 2 ++
blockdev.c | 5 +++++
include/block/block.h | 7 +++++++
include/block/block_int.h | 8 ++++++++
5 files changed, 36 insertions(+)
diff --git a/block.c b/block.c
index 4833b37..4da6fd9 100644
--- a/block.c
+++ b/block.c
@@ -4632,3 +4632,17 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
}
return bs->drv->bdrv_amend_options(bs, options);
}
+
+bool bdrv_check_ext_snapshot(BlockDriverState *bs)
+{
+ /* external snashots are enabled by defaults */
+ if (!bs->drv->bdrv_check_ext_snapshot) {
+ return true;
+ }
+ return bs->drv->bdrv_check_ext_snapshot(bs);
+}
+
+bool bdrv_forbid_ext_snapshot(BlockDriverState *bs)
+{
+ return false;
+}
diff --git a/block/blkverify.c b/block/blkverify.c
index 2077d8a..c548923 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -313,6 +313,8 @@ static BlockDriver bdrv_blkverify = {
.bdrv_aio_readv = blkverify_aio_readv,
.bdrv_aio_writev = blkverify_aio_writev,
.bdrv_aio_flush = blkverify_aio_flush,
+
+ .bdrv_check_ext_snapshot = bdrv_forbid_ext_snapshot,
};
static void bdrv_blkverify_init(void)
diff --git a/blockdev.c b/blockdev.c
index 8aa66a9..5c16f1b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1131,6 +1131,11 @@ static void external_snapshot_prepare(BlkTransactionState *common,
}
}
+ if (!bdrv_check_ext_snapshot(state->old_bs)) {
+ error_set(errp, QERR_FEATURE_DISABLED, "snapshot");
+ return;
+ }
+
flags = state->old_bs->open_flags;
/* create new image w/backing file */
diff --git a/include/block/block.h b/include/block/block.h
index f808550..df19610 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -244,6 +244,13 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
+/* external snapshots */
+
+/* return true if external snapshot is allowed, false if not */
+bool bdrv_check_ext_snapshot(BlockDriverState *bs);
+/* helper used to forbid external snapshots like in blkverify */
+bool bdrv_forbid_ext_snapshot(BlockDriverState *bs);
+
/* async block I/O */
typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
int sector_num);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 211087a..cb92355 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -67,6 +67,14 @@ typedef struct BdrvTrackedRequest {
struct BlockDriver {
const char *format_name;
int instance_size;
+
+ /* if not defined external snapshots are allowed
+ * if return true external snapshots are allowed
+ * if return false external snapshots are not allowed
+ * future block filters will query their children to build the response
+ */
+ bool (*bdrv_check_ext_snapshot)(BlockDriverState *bs);
+
int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
int (*bdrv_probe_device)(const char *filename);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH V2] block: Add BlockDriver.bdrv_check_ext_snapshot.
2013-09-26 14:33 ` [Qemu-devel] [PATCH V2] block: Add BlockDriver.bdrv_check_ext_snapshot Benoît Canet
@ 2013-09-26 16:48 ` Jeff Cody
2013-09-27 9:52 ` Kevin Wolf
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Cody @ 2013-09-26 16:48 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, qemu-devel, stefanha
On Thu, Sep 26, 2013 at 04:33:49PM +0200, Benoît Canet wrote:
> This field is used by blkverify to disable external snapshots creation.
> I will also be used by block filters like quorum to disable external snapshots
> creation.
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 14 ++++++++++++++
> block/blkverify.c | 2 ++
> blockdev.c | 5 +++++
> include/block/block.h | 7 +++++++
> include/block/block_int.h | 8 ++++++++
> 5 files changed, 36 insertions(+)
>
> diff --git a/block.c b/block.c
> index 4833b37..4da6fd9 100644
> --- a/block.c
> +++ b/block.c
> @@ -4632,3 +4632,17 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
> }
> return bs->drv->bdrv_amend_options(bs, options);
> }
> +
> +bool bdrv_check_ext_snapshot(BlockDriverState *bs)
> +{
> + /* external snashots are enabled by defaults */
> + if (!bs->drv->bdrv_check_ext_snapshot) {
> + return true;
> + }
> + return bs->drv->bdrv_check_ext_snapshot(bs);
> +}
> +
> +bool bdrv_forbid_ext_snapshot(BlockDriverState *bs)
> +{
> + return false;
> +}
The only problem I have with this now, is that
"bdrv_forbid_ext_snapshot()" returns false, to indicate that "forbid
ext snapshot" is true. Looking at the function above, I would come to
the opposite conclusion as to what it does.
I understand why - you want the function name assigned to
.bdrv_check_ext_snapshot to reflect the action, but then that causes
the boolean return to be misleading. Maybe returning an enum would be
more natural?
I apologize if this seems too pedantic. :)
Thanks,
Jeff
> diff --git a/block/blkverify.c b/block/blkverify.c
> index 2077d8a..c548923 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -313,6 +313,8 @@ static BlockDriver bdrv_blkverify = {
> .bdrv_aio_readv = blkverify_aio_readv,
> .bdrv_aio_writev = blkverify_aio_writev,
> .bdrv_aio_flush = blkverify_aio_flush,
> +
> + .bdrv_check_ext_snapshot = bdrv_forbid_ext_snapshot,
> };
>
> static void bdrv_blkverify_init(void)
> diff --git a/blockdev.c b/blockdev.c
> index 8aa66a9..5c16f1b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1131,6 +1131,11 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> }
> }
>
> + if (!bdrv_check_ext_snapshot(state->old_bs)) {
> + error_set(errp, QERR_FEATURE_DISABLED, "snapshot");
> + return;
> + }
> +
> flags = state->old_bs->open_flags;
>
> /* create new image w/backing file */
> diff --git a/include/block/block.h b/include/block/block.h
> index f808550..df19610 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -244,6 +244,13 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>
> int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
>
> +/* external snapshots */
> +
> +/* return true if external snapshot is allowed, false if not */
> +bool bdrv_check_ext_snapshot(BlockDriverState *bs);
> +/* helper used to forbid external snapshots like in blkverify */
> +bool bdrv_forbid_ext_snapshot(BlockDriverState *bs);
> +
> /* async block I/O */
> typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
> int sector_num);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 211087a..cb92355 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -67,6 +67,14 @@ typedef struct BdrvTrackedRequest {
> struct BlockDriver {
> const char *format_name;
> int instance_size;
> +
> + /* if not defined external snapshots are allowed
> + * if return true external snapshots are allowed
> + * if return false external snapshots are not allowed
> + * future block filters will query their children to build the response
> + */
> + bool (*bdrv_check_ext_snapshot)(BlockDriverState *bs);
> +
> int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
> int (*bdrv_probe_device)(const char *filename);
>
> --
> 1.8.1.2
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH V2] block: Add BlockDriver.bdrv_check_ext_snapshot.
2013-09-26 16:48 ` Jeff Cody
@ 2013-09-27 9:52 ` Kevin Wolf
0 siblings, 0 replies; 4+ messages in thread
From: Kevin Wolf @ 2013-09-27 9:52 UTC (permalink / raw)
To: Jeff Cody; +Cc: Benoît Canet, stefanha, qemu-devel
Am 26.09.2013 um 18:48 hat Jeff Cody geschrieben:
> On Thu, Sep 26, 2013 at 04:33:49PM +0200, Benoît Canet wrote:
> > This field is used by blkverify to disable external snapshots creation.
> > I will also be used by block filters like quorum to disable external snapshots
> > creation.
> >
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> > block.c | 14 ++++++++++++++
> > block/blkverify.c | 2 ++
> > blockdev.c | 5 +++++
> > include/block/block.h | 7 +++++++
> > include/block/block_int.h | 8 ++++++++
> > 5 files changed, 36 insertions(+)
> >
> > diff --git a/block.c b/block.c
> > index 4833b37..4da6fd9 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4632,3 +4632,17 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
> > }
> > return bs->drv->bdrv_amend_options(bs, options);
> > }
> > +
> > +bool bdrv_check_ext_snapshot(BlockDriverState *bs)
> > +{
> > + /* external snashots are enabled by defaults */
> > + if (!bs->drv->bdrv_check_ext_snapshot) {
> > + return true;
> > + }
> > + return bs->drv->bdrv_check_ext_snapshot(bs);
> > +}
> > +
> > +bool bdrv_forbid_ext_snapshot(BlockDriverState *bs)
> > +{
> > + return false;
> > +}
>
> The only problem I have with this now, is that
> "bdrv_forbid_ext_snapshot()" returns false, to indicate that "forbid
> ext snapshot" is true. Looking at the function above, I would come to
> the opposite conclusion as to what it does.
>
> I understand why - you want the function name assigned to
> .bdrv_check_ext_snapshot to reflect the action, but then that causes
> the boolean return to be misleading. Maybe returning an enum would be
> more natural?
>
> I apologize if this seems too pedantic. :)
Perhaps rename the function to bdrv_check_ext_snapshot_forbidden() or
something like that?
Kevin
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-27 9:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-26 14:33 [Qemu-devel] [PATCH V2] disable blkverify external snapshot creation Benoît Canet
2013-09-26 14:33 ` [Qemu-devel] [PATCH V2] block: Add BlockDriver.bdrv_check_ext_snapshot Benoît Canet
2013-09-26 16:48 ` Jeff Cody
2013-09-27 9:52 ` Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).