Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

2013-06-12 Thread Wenchao Xia 

于 2013-6-11 16:26, Stefan Hajnoczi 写道:

On Sat, Jun 08, 2013 at 02:58:00PM +0800, Wenchao Xia wrote:

+if (id  name) {
+for (i = 0; i  nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-id_str, id)  !strcmp(sn-name, name)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else if (id) {
+for (i = 0; i  nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-id_str, id)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else if (name) {
+for (i = 0; i  nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-name, name)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else {
+/* program error */
+abort();
+}


If you respin, this would be a little clearer:

assert(id || name);

if (id  name) {
 ...
} else if (id) {
 ...
} else if (name) {
 ...
}

The advantage is that the assert(3) condition is included in the error
message that gets printed.

Stefan


  OK, will do it in next version.

--
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

2013-06-11 Thread Stefan Hajnoczi 
On Sat, Jun 08, 2013 at 02:58:00PM +0800, Wenchao Xia wrote:
 +if (id  name) {
 +for (i = 0; i  nb_sns; i++) {
 +sn = sn_tab[i];
 +if (!strcmp(sn-id_str, id)  !strcmp(sn-name, name)) {
 +*sn_info = *sn;
 +ret = true;
 +break;
 +}
 +}
 +} else if (id) {
 +for (i = 0; i  nb_sns; i++) {
 +sn = sn_tab[i];
 +if (!strcmp(sn-id_str, id)) {
 +*sn_info = *sn;
 +ret = true;
 +break;
 +}
 +}
 +} else if (name) {
 +for (i = 0; i  nb_sns; i++) {
 +sn = sn_tab[i];
 +if (!strcmp(sn-name, name)) {
 +*sn_info = *sn;
 +ret = true;
 +break;
 +}
 +}
 +} else {
 +/* program error */
 +abort();
 +}

If you respin, this would be a little clearer:

assert(id || name);

if (id  name) {
...
} else if (id) {
...
} else if (name) {
...
}

The advantage is that the assert(3) condition is included in the error
message that gets printed.

Stefan

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

2013-06-08 Thread Fam Zheng 
On Sat, 06/08 14:58, Wenchao Xia wrote:
 To make it clear about id and name in searching, add this API
 to distinguish them. Caller can choose to search by id or name,
 *errp will be set only for exception.
 
 Some code are modified based on Pavel's patch.
 
 Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
 Signed-off-by: Pavel Hrdina phrd...@redhat.com
 ---
  block/snapshot.c |   74 
 ++
  include/block/snapshot.h |6 
  2 files changed, 80 insertions(+), 0 deletions(-)
 
 diff --git a/block/snapshot.c b/block/snapshot.c
 index 6c6d9de..0a9af4e 100644
 --- a/block/snapshot.c
 +++ b/block/snapshot.c
 @@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, 
 QEMUSnapshotInfo *sn_info,
  return ret;
  }
  
 +/**
 + * Look up an internal snapshot by @id and @name.
 + * @bs: block device to search
 + * @id: unique snapshot ID, or NULL
 + * @name: snapshot name, or NULL
 + * @sn_info: location to store information on the snapshot found
 + * @errp: location to store error, will be set only for exception
 + *
 + * This function will traverse snapshot list in @bs to search the matching
 + * one, @id and @name are the matching condition:
 + * If both @id and @name are specified, find the first one with id @id and
 + * name @name.
 + * If only @id is specified, find the first one with id @id.
 + * If only @name is specified, find the first one with name @name.
 + * if none is specified, abort().
 + *
 + * Returns: true when a snapshot is found and @sn_info will be filled, false
 + * when error or not found. If all operation succeed but no matching one is
 + * found, @errp will NOT be set.
 + */
 +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
 +   const char *id,
 +   const char *name,
 +   QEMUSnapshotInfo *sn_info,
 +   Error **errp)
 +{
 +QEMUSnapshotInfo *sn_tab, *sn;
 +int nb_sns, i;
 +bool ret = false;
 +
 +nb_sns = bdrv_snapshot_list(bs, sn_tab);
 +if (nb_sns  0) {
 +error_setg_errno(errp, -nb_sns, Failed to get a snapshot list);
 +return false;
 +} else if (nb_sns == 0) {
 +return false;
 +}
 +
 +if (id  name) {
 +for (i = 0; i  nb_sns; i++) {
 +sn = sn_tab[i];
 +if (!strcmp(sn-id_str, id)  !strcmp(sn-name, name)) {
 +*sn_info = *sn;
 +ret = true;
 +break;
 +}
 +}
 +} else if (id) {
 +for (i = 0; i  nb_sns; i++) {
 +sn = sn_tab[i];
 +if (!strcmp(sn-id_str, id)) {
 +*sn_info = *sn;
 +ret = true;
 +break;
 +}
 +}
 +} else if (name) {
 +for (i = 0; i  nb_sns; i++) {
 +sn = sn_tab[i];
 +if (!strcmp(sn-name, name)) {
 +*sn_info = *sn;
 +ret = true;
 +break;
 +}
 +}
 +} else {
 +/* program error */
 +abort();
 +}

Looks duplicated. How about:

if (id || name) {
for (i = 0; i  nb_sns; i++) {
sn = sn_tab[i];
if ((!id || !strcmp(sn-id_str, id)) 
(!name || !strcmp(sn-name, name))) {
*sn_info = *sn;
ret = true;
break;
}
}
} else {
abort();
}

And why do we have to abort here? It is not completely nonsense to me to
return first snapshot with id == NULL and name == NULL.

-- 
Fam

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

2013-06-08 Thread Wenchao Xia 

于 2013-6-8 15:31, Fam Zheng 写道:

On Sat, 06/08 14:58, Wenchao Xia wrote:

To make it clear about id and name in searching, add this API
to distinguish them. Caller can choose to search by id or name,
*errp will be set only for exception.

Some code are modified based on Pavel's patch.

Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
  block/snapshot.c |   74 ++
  include/block/snapshot.h |6 
  2 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..0a9af4e 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info,
  return ret;
  }

+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @sn_info: location to store information on the snapshot found
+ * @errp: location to store error, will be set only for exception
+ *
+ * This function will traverse snapshot list in @bs to search the matching
+ * one, @id and @name are the matching condition:
+ * If both @id and @name are specified, find the first one with id @id and
+ * name @name.
+ * If only @id is specified, find the first one with id @id.
+ * If only @name is specified, find the first one with name @name.
+ * if none is specified, abort().
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found. If all operation succeed but no matching one is
+ * found, @errp will NOT be set.
+ */
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+   const char *id,
+   const char *name,
+   QEMUSnapshotInfo *sn_info,
+   Error **errp)
+{
+QEMUSnapshotInfo *sn_tab, *sn;
+int nb_sns, i;
+bool ret = false;
+
+nb_sns = bdrv_snapshot_list(bs, sn_tab);
+if (nb_sns  0) {
+error_setg_errno(errp, -nb_sns, Failed to get a snapshot list);
+return false;
+} else if (nb_sns == 0) {
+return false;
+}
+
+if (id  name) {
+for (i = 0; i  nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-id_str, id)  !strcmp(sn-name, name)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else if (id) {
+for (i = 0; i  nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-id_str, id)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else if (name) {
+for (i = 0; i  nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-name, name)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else {
+/* program error */
+abort();
+}


Looks duplicated. How about:

 if (id || name) {
 for (i = 0; i  nb_sns; i++) {
 sn = sn_tab[i];
 if ((!id || !strcmp(sn-id_str, id)) 
 (!name || !strcmp(sn-name, name))) {
 *sn_info = *sn;
 ret = true;
 break;
 }
 }
 } else {
 abort();
 }


  Less code, but slightly slower since more if inside for. I think
three for also show more clear about judgement logic.


And why do we have to abort here? It is not completely nonsense to me to
return first snapshot with id == NULL and name == NULL.


  Just to tip program error. An snapshot with id == NULL and name ==
NULL is not possible, isn't it?.


--
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

2013-06-08 Thread Fam Zheng 
On Sat, 06/08 15:58, Wenchao Xia wrote:
 于 2013-6-8 15:31, Fam Zheng 写道:
 On Sat, 06/08 14:58, Wenchao Xia wrote:
 To make it clear about id and name in searching, add this API
 to distinguish them. Caller can choose to search by id or name,
 *errp will be set only for exception.
 
 Some code are modified based on Pavel's patch.
 
 Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
 Signed-off-by: Pavel Hrdina phrd...@redhat.com
 ---
   block/snapshot.c |   74 
  ++
   include/block/snapshot.h |6 
   2 files changed, 80 insertions(+), 0 deletions(-)
 
 diff --git a/block/snapshot.c b/block/snapshot.c
 index 6c6d9de..0a9af4e 100644
 --- a/block/snapshot.c
 +++ b/block/snapshot.c
 @@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, 
 QEMUSnapshotInfo *sn_info,
   return ret;
   }
 
 +/**
 + * Look up an internal snapshot by @id and @name.
 + * @bs: block device to search
 + * @id: unique snapshot ID, or NULL
 + * @name: snapshot name, or NULL
 + * @sn_info: location to store information on the snapshot found
 + * @errp: location to store error, will be set only for exception
 + *
 + * This function will traverse snapshot list in @bs to search the matching
 + * one, @id and @name are the matching condition:
 + * If both @id and @name are specified, find the first one with id @id and
 + * name @name.
 + * If only @id is specified, find the first one with id @id.
 + * If only @name is specified, find the first one with name @name.
 + * if none is specified, abort().
 + *
 + * Returns: true when a snapshot is found and @sn_info will be filled, 
 false
 + * when error or not found. If all operation succeed but no matching one is
 + * found, @errp will NOT be set.
 + */
 +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
 +   const char *id,
 +   const char *name,
 +   QEMUSnapshotInfo *sn_info,
 +   Error **errp)
 +{
 +QEMUSnapshotInfo *sn_tab, *sn;
 +int nb_sns, i;
 +bool ret = false;
 +
 +nb_sns = bdrv_snapshot_list(bs, sn_tab);
 +if (nb_sns  0) {
 +error_setg_errno(errp, -nb_sns, Failed to get a snapshot list);
 +return false;
 +} else if (nb_sns == 0) {
 +return false;
 +}
 +
 +if (id  name) {
 +for (i = 0; i  nb_sns; i++) {
 +sn = sn_tab[i];
 +if (!strcmp(sn-id_str, id)  !strcmp(sn-name, name)) {
 +*sn_info = *sn;
 +ret = true;
 +break;
 +}
 +}
 +} else if (id) {
 +for (i = 0; i  nb_sns; i++) {
 +sn = sn_tab[i];
 +if (!strcmp(sn-id_str, id)) {
 +*sn_info = *sn;
 +ret = true;
 +break;
 +}
 +}
 +} else if (name) {
 +for (i = 0; i  nb_sns; i++) {
 +sn = sn_tab[i];
 +if (!strcmp(sn-name, name)) {
 +*sn_info = *sn;
 +ret = true;
 +break;
 +}
 +}
 +} else {
 +/* program error */
 +abort();
 +}
 
 Looks duplicated. How about:
 
  if (id || name) {
  for (i = 0; i  nb_sns; i++) {
  sn = sn_tab[i];
  if ((!id || !strcmp(sn-id_str, id)) 
  (!name || !strcmp(sn-name, name))) {
  *sn_info = *sn;
  ret = true;
  break;
  }
  }
  } else {
  abort();
  }
 
   Less code, but slightly slower since more if inside for. I think
 three for also show more clear about judgement logic.

No I don't think if-in-for or for-in-if *here* makes any meaningful
difference in performance, if we really need it fast, we'd better sort the
list it first and binary search. And I don't see it clearer to duplicate
the same logic for three times, If I want to understand it, I need to
compare if#1 and if#2 to get they are the same, and then compare #2 and
#3 again, just to know that the three are no different.

 
 And why do we have to abort here? It is not completely nonsense to me to
 return first snapshot with id == NULL and name == NULL.
 
   Just to tip program error. An snapshot with id == NULL and name ==
 NULL is not possible, isn't it?.

OK.

-- 
Fam

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

2013-06-08 Thread Wenchao Xia 

于 2013-6-8 16:35, Fam Zheng 写道:

On Sat, 06/08 15:58, Wenchao Xia wrote:

于 2013-6-8 15:31, Fam Zheng 写道:

On Sat, 06/08 14:58, Wenchao Xia wrote:

To make it clear about id and name in searching, add this API
to distinguish them. Caller can choose to search by id or name,
*errp will be set only for exception.

Some code are modified based on Pavel's patch.

Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
  block/snapshot.c |   74 ++
  include/block/snapshot.h |6 
  2 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..0a9af4e 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info,
  return ret;
  }

+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @sn_info: location to store information on the snapshot found
+ * @errp: location to store error, will be set only for exception
+ *
+ * This function will traverse snapshot list in @bs to search the matching
+ * one, @id and @name are the matching condition:
+ * If both @id and @name are specified, find the first one with id @id and
+ * name @name.
+ * If only @id is specified, find the first one with id @id.
+ * If only @name is specified, find the first one with name @name.
+ * if none is specified, abort().
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found. If all operation succeed but no matching one is
+ * found, @errp will NOT be set.
+ */
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+   const char *id,
+   const char *name,
+   QEMUSnapshotInfo *sn_info,
+   Error **errp)
+{
+QEMUSnapshotInfo *sn_tab, *sn;
+int nb_sns, i;
+bool ret = false;
+
+nb_sns = bdrv_snapshot_list(bs, sn_tab);
+if (nb_sns  0) {
+error_setg_errno(errp, -nb_sns, Failed to get a snapshot list);
+return false;
+} else if (nb_sns == 0) {
+return false;
+}
+
+if (id  name) {
+for (i = 0; i  nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-id_str, id)  !strcmp(sn-name, name)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else if (id) {
+for (i = 0; i  nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-id_str, id)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else if (name) {
+for (i = 0; i  nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-name, name)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else {
+/* program error */
+abort();
+}


Looks duplicated. How about:

 if (id || name) {
 for (i = 0; i  nb_sns; i++) {
 sn = sn_tab[i];
 if ((!id || !strcmp(sn-id_str, id)) 
 (!name || !strcmp(sn-name, name))) {
 *sn_info = *sn;
 ret = true;
 break;
 }
 }
 } else {
 abort();
 }


   Less code, but slightly slower since more if inside for. I think
three for also show more clear about judgement logic.


No I don't think if-in-for or for-in-if *here* makes any meaningful
difference in performance, if we really need it fast, we'd better sort the

  My instinct is forbid if in for, just my custom.


list it first and binary search. And I don't see it clearer to duplicate
the same logic for three times, If I want to understand it, I need to

  Three cases makes work flow clear, and it is easy when I want to
change a logic in one case.


compare if#1 and if#2 to get they are the same, and then compare #2 and
#3 again, just to know that the three are no different.


  There is difference requiring reader think and extend it out into
three cases in his mind:
  if ((!id || !strcmp(sn-id_str, id)) 
  (!name || !strcmp(sn-name, name)))

It is a coding style issue, usually I don't mind to write more C lines 
to make things simple. Hope to get maintainer's idea.





And why do we have to abort here? It is not completely nonsense to me to
return first snapshot with id == NULL and name == NULL.


   Just to tip program error. An snapshot with id == NULL and name ==
NULL is not possible, isn't it?.


OK.




--
Best Regards

Wenchao Xia