Re: [Qemu-devel] [PATCH V17 11/12] quorum: Add quorum_open() and quorum_close().

2014-02-18 Thread Benoît Canet 
The Tuesday 18 Feb 2014 à 12:06:57 (+0800), Fam Zheng wrote :
 On Thu, 02/13 10:09, Benoît Canet wrote:
  The Wednesday 12 Feb 2014 à 23:06:38 (+0100), Benoît Canet wrote :
   +static void quorum_close(BlockDriverState *bs)
   +{
   +BDRVQuorumState *s = bs-opaque;
   +int i;
   +
   +for (i = 0; i  s-num_children; i++) {
   +bdrv_unref(s-bs[i]);
  Quorum crash here from time to time I don't understand why.
 
 I think you could add printf or use gdb to examine every bdrv_unref on the
 crashing bs, so you can track down to the unbalanced reference.

I found the cause of the crash since: I added a spurious QDECREF() in the
external snapshot prepare function.
It's fixed now.

Thanks,

Benoît

 
 Fam

Re: [Qemu-devel] [PATCH V17 11/12] quorum: Add quorum_open() and quorum_close().

2014-02-17 Thread Fam Zheng 
On Thu, 02/13 10:09, Benoît Canet wrote:
 The Wednesday 12 Feb 2014 à 23:06:38 (+0100), Benoît Canet wrote :
  +static void quorum_close(BlockDriverState *bs)
  +{
  +BDRVQuorumState *s = bs-opaque;
  +int i;
  +
  +for (i = 0; i  s-num_children; i++) {
  +bdrv_unref(s-bs[i]);
 Quorum crash here from time to time I don't understand why.

I think you could add printf or use gdb to examine every bdrv_unref on the
crashing bs, so you can track down to the unbalanced reference.

Fam

Re: [Qemu-devel] [PATCH V17 11/12] quorum: Add quorum_open() and quorum_close().

2014-02-14 Thread Max Reitz 

On 12.02.2014 23:06, Benoît Canet wrote:

From: Benoît Canet ben...@irqsave.net

Example of command line:
-drive if=virtio,file.driver=quorum,\
file.children.0.file.filename=1.raw,\
file.children.0.node-name=1.raw,\
file.children.0.driver=raw,\
file.children.1.file.filename=2.raw,\
file.children.1.node-name=2.raw,\
file.children.1.driver=raw,\
file.children.2.file.filename=3.raw,\
file.children.2.node-name=3.raw,\
file.children.2.driver=raw,\
file.vote-threshold=2

file.blkverify=on with file.vote-threshold=2 and two files can be passed to
emulated blkverify.

Signed-off-by: Benoit Canet ben...@irqsave.net
---
  block/quorum.c   | 165 +++
  monitor.c|   2 +
  qapi-schema.json |  21 ++-
  3 files changed, 187 insertions(+), 1 deletion(-)

diff --git a/block/quorum.c b/block/quorum.c
index 8e033ef..d3a90fb 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -20,6 +20,9 @@
  
  #define HASH_LENGTH 32
  
+#define QUORUM_OPT_VOTE_THRESHOLD vote-threshold

+#define QUORUM_OPT_BLKVERIFY  blkverify
+
  /* This union holds a vote hash value */
  typedef union QuorumVoteValue {
  char h[HASH_LENGTH];   /* SHA-256 hash */
@@ -677,12 +680,174 @@ static bool 
quorum_recurse_is_first_non_filter(BlockDriverState *bs,
  return false;
  }
  
+static int quorum_valid_threshold(int threshold,

+  int num_children,
+  Error **errp)


This fits on a single line and I'm all for maximizing usage of the 80 
character limit.



+{
+
+if (threshold  1) {
+error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+  vote-threshold, value = 1);
+return -ERANGE;
+}
+
+if (threshold  num_children) {
+error_setg(errp, threshold may not exceed children count);
+return -ERANGE;
+}
+
+return 0;
+}
+
+static QemuOptsList quorum_runtime_opts = {
+.name = quorum,
+.head = QTAILQ_HEAD_INITIALIZER(quorum_runtime_opts.head),
+.desc = {
+{
+.name = QUORUM_OPT_VOTE_THRESHOLD,
+.type = QEMU_OPT_NUMBER,
+.help = The number of vote needed for reaching quorum,
+},
+{
+.name = QUORUM_OPT_BLKVERIFY,
+.type = QEMU_OPT_BOOL,
+.help = Trigger block verify mode if set,
+},
+{ /* end of list */ }
+},
+};
+
+static int quorum_open(BlockDriverState *bs,
+   QDict *options,
+   int flags,
+   Error **errp)


This takes two lines (although since you have to split at all, I can 
cope with this being split once per parameter).



+{
+BDRVQuorumState *s = bs-opaque;
+Error *local_err = NULL;
+QemuOpts *opts;
+bool *opened;
+QDict *sub = NULL;
+QList *list = NULL;
+const QListEntry *lentry;
+const QDictEntry *dentry;
+int i;
+int ret = 0;
+
+qdict_flatten(options);
+qdict_extract_subqdict(options, sub, children.);
+qdict_array_split(sub, list);
+
+/* count how many different children are present and validate
+ * qdict_size(sub) address the open by reference case
+ */
+s-num_children = !qlist_size(list) ? qdict_size(sub) : qlist_size(list);


Okay, so eventually we probably want to add these values in order to 
allow mixed parameters (some references, some full specifications). 
But for that to work without much hassle, qdict_array_split() still 
needs some extension (or be fixed?) to split not just QDicts but other 
objects as well. I think we (probably I) should amend 
qdict_array_split() first (perhaps add some boolean parameter to specify 
what it should do) and then fix this (that is, make mixed parameters 
work and allow detection of malformed parameters).


For this series, this is fine.


+if (s-num_children  2) {
+error_setg(local_err,
+   Number of provided children must be greater than 1);
+ret = -EINVAL;
+goto exit;
+}
+
+opts = qemu_opts_create(quorum_runtime_opts, NULL, 0, error_abort);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+ret = -EINVAL;
+goto exit;
+}
+
+s-threshold = qemu_opt_get_number(opts, QUORUM_OPT_VOTE_THRESHOLD, 0);
+
+/* and validate it against s-num_children */
+ret = quorum_valid_threshold(s-threshold, s-num_children, local_err);
+if (ret  0) {
+goto exit;
+}
+
+/* is the driver in blkverify mode */
+if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false) 
+s-num_children == 2  s-threshold == 2) {


If qemu_opt_get_bool() returns true but the other conditions aren't met, 
I'd prefer, again, a warning.



+s-is_blkverify = true;
+}
+
+/* allocate the children BlockDriverState array */
+s-bs = g_new0(BlockDriverState *, s-num_children);
+opened = g_new0(bool, s-num_children);
+
+/* Open by file

Re: [Qemu-devel] [PATCH V17 11/12] quorum: Add quorum_open() and quorum_close().

2014-02-13 Thread Benoît Canet 
The Wednesday 12 Feb 2014 à 23:06:38 (+0100), Benoît Canet wrote :
 From: Benoît Canet ben...@irqsave.net
 
 Example of command line:
 -drive if=virtio,file.driver=quorum,\
 file.children.0.file.filename=1.raw,\
 file.children.0.node-name=1.raw,\
 file.children.0.driver=raw,\
 file.children.1.file.filename=2.raw,\
 file.children.1.node-name=2.raw,\
 file.children.1.driver=raw,\
 file.children.2.file.filename=3.raw,\
 file.children.2.node-name=3.raw,\
 file.children.2.driver=raw,\
 file.vote-threshold=2

Extra file. are wrong.

 
 file.blkverify=on with file.vote-threshold=2 and two files can be passed to
 emulated blkverify.
 
 Signed-off-by: Benoit Canet ben...@irqsave.net
 ---
  block/quorum.c   | 165 
 +++
  monitor.c|   2 +
  qapi-schema.json |  21 ++-
  3 files changed, 187 insertions(+), 1 deletion(-)
 
 diff --git a/block/quorum.c b/block/quorum.c
 index 8e033ef..d3a90fb 100644
 --- a/block/quorum.c
 +++ b/block/quorum.c
 @@ -20,6 +20,9 @@
  
  #define HASH_LENGTH 32
  
 +#define QUORUM_OPT_VOTE_THRESHOLD vote-threshold
 +#define QUORUM_OPT_BLKVERIFY  blkverify
 +
  /* This union holds a vote hash value */
  typedef union QuorumVoteValue {
  char h[HASH_LENGTH];   /* SHA-256 hash */
 @@ -677,12 +680,174 @@ static bool 
 quorum_recurse_is_first_non_filter(BlockDriverState *bs,
  return false;
  }
  
 +static int quorum_valid_threshold(int threshold,
 +  int num_children,
 +  Error **errp)
 +{
 +
 +if (threshold  1) {
 +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
 +  vote-threshold, value = 1);
 +return -ERANGE;
 +}
 +
 +if (threshold  num_children) {
 +error_setg(errp, threshold may not exceed children count);
 +return -ERANGE;
 +}
 +
 +return 0;
 +}
 +
 +static QemuOptsList quorum_runtime_opts = {
 +.name = quorum,
 +.head = QTAILQ_HEAD_INITIALIZER(quorum_runtime_opts.head),
 +.desc = {
 +{
 +.name = QUORUM_OPT_VOTE_THRESHOLD,
 +.type = QEMU_OPT_NUMBER,
 +.help = The number of vote needed for reaching quorum,
 +},
 +{
 +.name = QUORUM_OPT_BLKVERIFY,
 +.type = QEMU_OPT_BOOL,
 +.help = Trigger block verify mode if set,
 +},
 +{ /* end of list */ }
 +},
 +};
 +
 +static int quorum_open(BlockDriverState *bs,
 +   QDict *options,
 +   int flags,
 +   Error **errp)
 +{
 +BDRVQuorumState *s = bs-opaque;
 +Error *local_err = NULL;
 +QemuOpts *opts;
 +bool *opened;
 +QDict *sub = NULL;
 +QList *list = NULL;
 +const QListEntry *lentry;
 +const QDictEntry *dentry;
 +int i;
 +int ret = 0;
 +
 +qdict_flatten(options);
 +qdict_extract_subqdict(options, sub, children.);
 +qdict_array_split(sub, list);
 +
 +/* count how many different children are present and validate
 + * qdict_size(sub) address the open by reference case
 + */
 +s-num_children = !qlist_size(list) ? qdict_size(sub) : qlist_size(list);
 +if (s-num_children  2) {
 +error_setg(local_err,
 +   Number of provided children must be greater than 1);
 +ret = -EINVAL;
 +goto exit;
 +}
 +
 +opts = qemu_opts_create(quorum_runtime_opts, NULL, 0, error_abort);
 +qemu_opts_absorb_qdict(opts, options, local_err);
 +if (error_is_set(local_err)) {
 +ret = -EINVAL;
 +goto exit;
 +}
 +
 +s-threshold = qemu_opt_get_number(opts, QUORUM_OPT_VOTE_THRESHOLD, 0);
 +
 +/* and validate it against s-num_children */
 +ret = quorum_valid_threshold(s-threshold, s-num_children, local_err);
 +if (ret  0) {
 +goto exit;
 +}
 +
 +/* is the driver in blkverify mode */
 +if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false) 
 +s-num_children == 2  s-threshold == 2) {
 +s-is_blkverify = true;
 +}
 +
 +/* allocate the children BlockDriverState array */
 +s-bs = g_new0(BlockDriverState *, s-num_children);
 +opened = g_new0(bool, s-num_children);
 +
 +/* Open by file name or options dict (command line or QMP) */
 +if (s-num_children == qlist_size(list)) {
 +for (i = 0, lentry = qlist_first(list); lentry;
 +lentry = qlist_next(lentry), i++) {
 +QDict *d = qobject_to_qdict(lentry-value);
 +QINCREF(d);
 +ret = bdrv_open(s-bs[i], NULL, NULL,
 +d,
 +flags, NULL, local_err);
 +if (ret  0) {
 +goto close_exit;
 +}
 +opened[i] = true;
 +}
 +/* Open by QMP references */
 +} else {
 +for (i = 0, dentry = qdict_first(sub); dentry;
 + dentry = qdict_next(sub, dentry), i++) {
 +

[Qemu-devel] [PATCH V17 11/12] quorum: Add quorum_open() and quorum_close().

2014-02-12 Thread Benoît Canet 
From: Benoît Canet ben...@irqsave.net

Example of command line:
-drive if=virtio,file.driver=quorum,\
file.children.0.file.filename=1.raw,\
file.children.0.node-name=1.raw,\
file.children.0.driver=raw,\
file.children.1.file.filename=2.raw,\
file.children.1.node-name=2.raw,\
file.children.1.driver=raw,\
file.children.2.file.filename=3.raw,\
file.children.2.node-name=3.raw,\
file.children.2.driver=raw,\
file.vote-threshold=2

file.blkverify=on with file.vote-threshold=2 and two files can be passed to
emulated blkverify.

Signed-off-by: Benoit Canet ben...@irqsave.net
---
 block/quorum.c   | 165 +++
 monitor.c|   2 +
 qapi-schema.json |  21 ++-
 3 files changed, 187 insertions(+), 1 deletion(-)

diff --git a/block/quorum.c b/block/quorum.c
index 8e033ef..d3a90fb 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -20,6 +20,9 @@
 
 #define HASH_LENGTH 32
 
+#define QUORUM_OPT_VOTE_THRESHOLD vote-threshold
+#define QUORUM_OPT_BLKVERIFY  blkverify
+
 /* This union holds a vote hash value */
 typedef union QuorumVoteValue {
 char h[HASH_LENGTH];   /* SHA-256 hash */
@@ -677,12 +680,174 @@ static bool 
quorum_recurse_is_first_non_filter(BlockDriverState *bs,
 return false;
 }
 
+static int quorum_valid_threshold(int threshold,
+  int num_children,
+  Error **errp)
+{
+
+if (threshold  1) {
+error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+  vote-threshold, value = 1);
+return -ERANGE;
+}
+
+if (threshold  num_children) {
+error_setg(errp, threshold may not exceed children count);
+return -ERANGE;
+}
+
+return 0;
+}
+
+static QemuOptsList quorum_runtime_opts = {
+.name = quorum,
+.head = QTAILQ_HEAD_INITIALIZER(quorum_runtime_opts.head),
+.desc = {
+{
+.name = QUORUM_OPT_VOTE_THRESHOLD,
+.type = QEMU_OPT_NUMBER,
+.help = The number of vote needed for reaching quorum,
+},
+{
+.name = QUORUM_OPT_BLKVERIFY,
+.type = QEMU_OPT_BOOL,
+.help = Trigger block verify mode if set,
+},
+{ /* end of list */ }
+},
+};
+
+static int quorum_open(BlockDriverState *bs,
+   QDict *options,
+   int flags,
+   Error **errp)
+{
+BDRVQuorumState *s = bs-opaque;
+Error *local_err = NULL;
+QemuOpts *opts;
+bool *opened;
+QDict *sub = NULL;
+QList *list = NULL;
+const QListEntry *lentry;
+const QDictEntry *dentry;
+int i;
+int ret = 0;
+
+qdict_flatten(options);
+qdict_extract_subqdict(options, sub, children.);
+qdict_array_split(sub, list);
+
+/* count how many different children are present and validate
+ * qdict_size(sub) address the open by reference case
+ */
+s-num_children = !qlist_size(list) ? qdict_size(sub) : qlist_size(list);
+if (s-num_children  2) {
+error_setg(local_err,
+   Number of provided children must be greater than 1);
+ret = -EINVAL;
+goto exit;
+}
+
+opts = qemu_opts_create(quorum_runtime_opts, NULL, 0, error_abort);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+ret = -EINVAL;
+goto exit;
+}
+
+s-threshold = qemu_opt_get_number(opts, QUORUM_OPT_VOTE_THRESHOLD, 0);
+
+/* and validate it against s-num_children */
+ret = quorum_valid_threshold(s-threshold, s-num_children, local_err);
+if (ret  0) {
+goto exit;
+}
+
+/* is the driver in blkverify mode */
+if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false) 
+s-num_children == 2  s-threshold == 2) {
+s-is_blkverify = true;
+}
+
+/* allocate the children BlockDriverState array */
+s-bs = g_new0(BlockDriverState *, s-num_children);
+opened = g_new0(bool, s-num_children);
+
+/* Open by file name or options dict (command line or QMP) */
+if (s-num_children == qlist_size(list)) {
+for (i = 0, lentry = qlist_first(list); lentry;
+lentry = qlist_next(lentry), i++) {
+QDict *d = qobject_to_qdict(lentry-value);
+QINCREF(d);
+ret = bdrv_open(s-bs[i], NULL, NULL,
+d,
+flags, NULL, local_err);
+if (ret  0) {
+goto close_exit;
+}
+opened[i] = true;
+}
+/* Open by QMP references */
+} else {
+for (i = 0, dentry = qdict_first(sub); dentry;
+ dentry = qdict_next(sub, dentry), i++) {
+QString *string = qobject_to_qstring(dentry-value);
+ret = bdrv_open(s-bs[i], NULL,
+qstring_get_str(string),
+NULL, flags, NULL, local_err);
+if (ret  0) {
+