At 2015/7/2 22:02, Alberto Garcia Wrote:
On Tue 30 Jun 2015 05:34:30 AM CEST, Wen Congyang wrote:
+static void quorum_add_child(BlockDriverState *bs, QDict *options, Error
**errp)
+{
+ BDRVQuorumState *s = bs->opaque;
+ int ret;
+ Error *local_err = NULL;
+
+ bdrv_drain(bs);
+
+ if (s->num_children == s->max_children) {
+ if (s->max_children >= INT_MAX / 2) {
+ error_setg(errp, "Too many children");
+ return;
+ }
+
+ s->bs = g_renew(BlockDriverState *, s->bs, s->max_children * 2);
+ memset(&s->bs[s->max_children], 0, s->max_children * sizeof(void *));
+ s->max_children *= 2;
+ }
+
+ ret = bdrv_open_image(&s->bs[s->num_children], NULL, options, "child", bs,
+ &child_format, false, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ s->num_children++;
+
+ /* TODO: Update vote_threshold */
+}
A few comments:
1) Is there any reason why you grow the array exponentially instead of
adding a fixed number of elements when the limit is reached? I guess
in practice the number of children is never going to be too high
anyway...
Yes, will do it in the next version.
2) Did you think of any API to update vote_threshold? Currently it
cannot be higher than num_children, so it's effectively limited by
the number of children that are attached when the quorum device is
created.
The things I think about it is: if vote_threshold-- when deleting a
child, vote_threshold
will be less than 0. If we don't do vote_threshold-- when it is 1, the
vote_threshold will
be changed when all children are added again. Which behavior is better?
3) I don't think it's necessary to set to NULL the pointers in s->bs[i]
when i >= num_children. There's no way to access those pointers
anyway. Same for the ' s->bs[s->num_children] = NULL; ' bit in
quorum_del_child(). I also think that using memset() for setting NULL
pointers is not portable, although QEMU is already doing this in a
few places.
OK, will remove it in the next version. Just a question: why is using
memset()
for setting NULL pointers is not prtable?
Thanks
Wen Congyang
Otherwise the code looks good to me. Thanks!
Berto
