| Message ID | 1373270451-18436-3-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben: > Internal snapshot's ID and name concept are both visible in general > block level, they are observed by user in "info snapshots", so it is > possible to have conflict. Although we can separate the two concept in > programming, but if they can be distinguished in string itself, things > will be simple and clear, so introduce two functions to do it. > > The implemention, qcow2 snapshot calls snapshot_id_string_generate() to > make sure it follows the rule in driver. If caller or user give a check > with snapshot_name_wellformed() before create snapshot, then the ID > and name will never conflict. The check can be also taken in > qcow2_snapshot_create(), but require it to return error reason. I'm not sure how useful this is. While we can restrict what IDs we allow for creating new snapshots, we cannot take any advantage from it because existing snapshots could already be named with only digits (they could also use a non-numeric ID). At the end of the day we're limiting the choice of IDs that can be generated with the QMP command without a real reason. Kevin
于 2013-7-10 21:10, Kevin Wolf 写道: > Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben: >> Internal snapshot's ID and name concept are both visible in general >> block level, they are observed by user in "info snapshots", so it is >> possible to have conflict. Although we can separate the two concept in >> programming, but if they can be distinguished in string itself, things >> will be simple and clear, so introduce two functions to do it. >> >> The implemention, qcow2 snapshot calls snapshot_id_string_generate() to >> make sure it follows the rule in driver. If caller or user give a check >> with snapshot_name_wellformed() before create snapshot, then the ID >> and name will never conflict. The check can be also taken in >> qcow2_snapshot_create(), but require it to return error reason. > > I'm not sure how useful this is. While we can restrict what IDs we allow > for creating new snapshots, we cannot take any advantage from it because > existing snapshots could already be named with only digits (they could > also use a non-numeric ID). At the end of the day we're limiting the Qcow2's ID seems always numeric, do you mean sheepdog may have non-numeric ID? > choice of IDs that can be generated with the QMP command without a real > reason. > This patch may limit the choice of snapshot name. Maybe we can't benefit from it now because existing code, but shouldn't it be improved in any new interface to alleviate the problem? I guess you idea is drop this check in the new interface, isn't it? > Kevin >
Am 10.07.2013 um 15:54 hat Wenchao Xia geschrieben: > 于 2013-7-10 21:10, Kevin Wolf 写道: > >Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben: > >>Internal snapshot's ID and name concept are both visible in general > >>block level, they are observed by user in "info snapshots", so it is > >>possible to have conflict. Although we can separate the two concept in > >>programming, but if they can be distinguished in string itself, things > >>will be simple and clear, so introduce two functions to do it. > >> > >>The implemention, qcow2 snapshot calls snapshot_id_string_generate() to > >>make sure it follows the rule in driver. If caller or user give a check > >>with snapshot_name_wellformed() before create snapshot, then the ID > >>and name will never conflict. The check can be also taken in > >>qcow2_snapshot_create(), but require it to return error reason. > > > >I'm not sure how useful this is. While we can restrict what IDs we allow > >for creating new snapshots, we cannot take any advantage from it because > >existing snapshots could already be named with only digits (they could > >also use a non-numeric ID). At the end of the day we're limiting the > Qcow2's ID seems always numeric, do you mean sheepdog may have > non-numeric ID? That's not true, qcow2 has an ID string rather than a numeric ID. > >choice of IDs that can be generated with the QMP command without a real > >reason. > > > This patch may limit the choice of snapshot name. Maybe we can't > benefit from it now because existing code, but shouldn't it be improved > in any new interface to alleviate the problem? I guess you idea > is drop this check in the new interface, isn't it? Well, the thought is that limiting it in QMP is of no use if there is no corresponding limitation in the backend. So yes, I think, as long as block drivers can deal with numeric snapshots names, QMP should allow dealing with them. Kevin
于 2013-7-10 22:22, Kevin Wolf 写道: > Am 10.07.2013 um 15:54 hat Wenchao Xia geschrieben: >> 于 2013-7-10 21:10, Kevin Wolf 写道: >>> Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben: >>>> Internal snapshot's ID and name concept are both visible in general >>>> block level, they are observed by user in "info snapshots", so it is >>>> possible to have conflict. Although we can separate the two concept in >>>> programming, but if they can be distinguished in string itself, things >>>> will be simple and clear, so introduce two functions to do it. >>>> >>>> The implemention, qcow2 snapshot calls snapshot_id_string_generate() to >>>> make sure it follows the rule in driver. If caller or user give a check >>>> with snapshot_name_wellformed() before create snapshot, then the ID >>>> and name will never conflict. The check can be also taken in >>>> qcow2_snapshot_create(), but require it to return error reason. >>> >>> I'm not sure how useful this is. While we can restrict what IDs we allow >>> for creating new snapshots, we cannot take any advantage from it because >>> existing snapshots could already be named with only digits (they could >>> also use a non-numeric ID). At the end of the day we're limiting the >> Qcow2's ID seems always numeric, do you mean sheepdog may have >> non-numeric ID? > > That's not true, qcow2 has an ID string rather than a numeric ID. > But there is no way to specify non-numeric ID in public interface, such as savevm, qemu-img, and in qcow2_snapshot_create(), find_new_snapshot_id() will always fill in a numeric ID string, so result is that, qcow2's snapshot ID string is always a numeric string. >>> choice of IDs that can be generated with the QMP command without a real >>> reason. >>> >> This patch may limit the choice of snapshot name. Maybe we can't >> benefit from it now because existing code, but shouldn't it be improved >> in any new interface to alleviate the problem? I guess you idea >> is drop this check in the new interface, isn't it? > > Well, the thought is that limiting it in QMP is of no use if there is no > corresponding limitation in the backend. So yes, I think, as long as > block drivers can deal with numeric snapshots names, QMP should allow > dealing with them. > OK, I'll drop this limit. > Kevin >
Am 10.07.2013 um 16:43 hat Wenchao Xia geschrieben: > 于 2013-7-10 22:22, Kevin Wolf 写道: > >Am 10.07.2013 um 15:54 hat Wenchao Xia geschrieben: > >>于 2013-7-10 21:10, Kevin Wolf 写道: > >>>Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben: > >>>>Internal snapshot's ID and name concept are both visible in general > >>>>block level, they are observed by user in "info snapshots", so it is > >>>>possible to have conflict. Although we can separate the two concept in > >>>>programming, but if they can be distinguished in string itself, things > >>>>will be simple and clear, so introduce two functions to do it. > >>>> > >>>>The implemention, qcow2 snapshot calls snapshot_id_string_generate() to > >>>>make sure it follows the rule in driver. If caller or user give a check > >>>>with snapshot_name_wellformed() before create snapshot, then the ID > >>>>and name will never conflict. The check can be also taken in > >>>>qcow2_snapshot_create(), but require it to return error reason. > >>> > >>>I'm not sure how useful this is. While we can restrict what IDs we allow > >>>for creating new snapshots, we cannot take any advantage from it because > >>>existing snapshots could already be named with only digits (they could > >>>also use a non-numeric ID). At the end of the day we're limiting the > >> Qcow2's ID seems always numeric, do you mean sheepdog may have > >>non-numeric ID? > > > >That's not true, qcow2 has an ID string rather than a numeric ID. > > > But there is no way to specify non-numeric ID in public interface, > such as savevm, qemu-img, and in qcow2_snapshot_create(), > find_new_snapshot_id() will always fill in a numeric ID string, so > result is that, qcow2's snapshot ID string is always a numeric string. Who said that qemu is the only program on the world that can create qcow2 snapshots? It might be, I don't know of any other, but qcow2 is defined by its spec, not by qemu's source code. And the spec talks about an ID string (just like the qemu code does, even though it happens to only write numbers into this string). Kevin
于 2013-7-10 22:49, Kevin Wolf 写道: > Am 10.07.2013 um 16:43 hat Wenchao Xia geschrieben: >> 于 2013-7-10 22:22, Kevin Wolf 写道: >>> Am 10.07.2013 um 15:54 hat Wenchao Xia geschrieben: >>>> 于 2013-7-10 21:10, Kevin Wolf 写道: >>>>> Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben: >>>>>> Internal snapshot's ID and name concept are both visible in general >>>>>> block level, they are observed by user in "info snapshots", so it is >>>>>> possible to have conflict. Although we can separate the two concept in >>>>>> programming, but if they can be distinguished in string itself, things >>>>>> will be simple and clear, so introduce two functions to do it. >>>>>> >>>>>> The implemention, qcow2 snapshot calls snapshot_id_string_generate() to >>>>>> make sure it follows the rule in driver. If caller or user give a check >>>>>> with snapshot_name_wellformed() before create snapshot, then the ID >>>>>> and name will never conflict. The check can be also taken in >>>>>> qcow2_snapshot_create(), but require it to return error reason. >>>>> >>>>> I'm not sure how useful this is. While we can restrict what IDs we allow >>>>> for creating new snapshots, we cannot take any advantage from it because >>>>> existing snapshots could already be named with only digits (they could >>>>> also use a non-numeric ID). At the end of the day we're limiting the >>>> Qcow2's ID seems always numeric, do you mean sheepdog may have >>>> non-numeric ID? >>> >>> That's not true, qcow2 has an ID string rather than a numeric ID. >>> >> But there is no way to specify non-numeric ID in public interface, >> such as savevm, qemu-img, and in qcow2_snapshot_create(), >> find_new_snapshot_id() will always fill in a numeric ID string, so >> result is that, qcow2's snapshot ID string is always a numeric string. > > Who said that qemu is the only program on the world that can create > qcow2 snapshots? It might be, I don't know of any other, but qcow2 is > defined by its spec, not by qemu's source code. And the spec talks about > an ID string (just like the qemu code does, even though it happens to > only write numbers into this string). > > Kevin > OK, it make sense.
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 0caac90..4f9c524 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -285,7 +285,7 @@ static void find_new_snapshot_id(BlockDriverState *bs, if (id > id_max) id_max = id; } - snprintf(id_str, id_str_size, "%d", id_max + 1); + snapshot_id_string_generate(id_max + 1, id_str, id_str_size); } static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str) diff --git a/block/snapshot.c b/block/snapshot.c index 481a3ee..40e0cc7 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -228,3 +228,24 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs, } return -ENOTSUP; } + +/* + * Return true if the given internal snapshot name is valid, false + * otherwise. + * + * To prevent clashes with internal snapshot IDs, names consisting only + * of digits are rejected. Empty strings are also rejected. + */ +bool snapshot_name_wellformed(const char *name) +{ + return strspn(name, "0123456789") != strlen(name); +} + +/* + * Following function generate id string, used by block driver, such as qcow2. + * Since no better place to go, place the funtion here for now. + */ +void snapshot_id_string_generate(int id, char *id_str, int id_str_size) +{ + snprintf(id_str, id_str_size, "%d", id); +} diff --git a/include/block/snapshot.h b/include/block/snapshot.h index 9d06dc1..3d93719 100644 --- a/include/block/snapshot.h +++ b/include/block/snapshot.h @@ -56,4 +56,7 @@ int bdrv_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_info); int bdrv_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name); + +bool snapshot_name_wellformed(const char *name); +void snapshot_id_string_generate(int id, char *id_str, int id_str_size); #endif
Internal snapshot's ID and name concept are both visible in general block level, they are observed by user in "info snapshots", so it is possible to have conflict. Although we can separate the two concept in programming, but if they can be distinguished in string itself, things will be simple and clear, so introduce two functions to do it. The implemention, qcow2 snapshot calls snapshot_id_string_generate() to make sure it follows the rule in driver. If caller or user give a check with snapshot_name_wellformed() before create snapshot, then the ID and name will never conflict. The check can be also taken in qcow2_snapshot_create(), but require it to return error reason. For rbd, it have no ID, so have no impact. For sheepdog, ID can't be determined in qemu, so still can't guarantee that no more conflict for ID and name. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- block/qcow2-snapshot.c | 2 +- block/snapshot.c | 21 +++++++++++++++++++++ include/block/snapshot.h | 3 +++ 3 files changed, 25 insertions(+), 1 deletions(-)