[05/11] snapshot: add paired functions for internal snapshot id and name

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 mess them up. 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 mess up. 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 mess up for ID and name.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c   |    2 +-
 block/snapshot.c         |   33 +++++++++++++++++++++++++++++++++
 include/block/snapshot.h |    3 +++
 3 files changed, 37 insertions(+), 1 deletions(-)

On Sat, Jun 08, 2013 at 02:58:01PM +0800, Wenchao Xia wrote:
> +/*
> + * Every internal snapshot have an ID used by qemu block layer, this function
> + * check whether name used by user mess up with ID. An empty string is also
> + * invalid.
> + */
> +bool snapshot_name_wellformed(const char *name)
> +{
> +    char *p;
> +    /* variable v is used to remove gcc warning of "ignoring return value" and
> +       "set but not used" */
> +    unsigned long v;
> +
> +    if (*name == '\0') {
> +        return false;
> +    }
> +
> +    v = strtoul(name, &p, 10);
> +    v++;
> +
> +    if (*p == '\0') {
> +        /* Numeric string */
> +        return false;
> +    }
> +    return true;
> +}

Shorter function with the same behavior and a rewritten doc comment:

/*
 * 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);
> +}

Since the caller has to manage id, this function doesn't really abstract
anything.  I would keep the snprintf() inline, there's only one caller.

于 2013-6-11 17:14, Stefan Hajnoczi 写道:
> On Sat, Jun 08, 2013 at 02:58:01PM +0800, Wenchao Xia wrote:
>> +/*
>> + * Every internal snapshot have an ID used by qemu block layer, this function
>> + * check whether name used by user mess up with ID. An empty string is also
>> + * invalid.
>> + */
>> +bool snapshot_name_wellformed(const char *name)
>> +{
>> +    char *p;
>> +    /* variable v is used to remove gcc warning of "ignoring return value" and
>> +       "set but not used" */
>> +    unsigned long v;
>> +
>> +    if (*name == '\0') {
>> +        return false;
>> +    }
>> +
>> +    v = strtoul(name, &p, 10);
>> +    v++;
>> +
>> +    if (*p == '\0') {
>> +        /* Numeric string */
>> +        return false;
>> +    }
>> +    return true;
>> +}
>
> Shorter function with the same behavior and a rewritten doc comment:
>
> /*
>   * 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);
> }
>
   much nicer, will use it, thanks!

>> +
>> +/* 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);
>> +}
>
> Since the caller has to manage id, this function doesn't really abstract
> anything.  I would keep the snprintf() inline, there's only one caller.
>
   Its purpose is move the id rules from one implemention(qcow2) into
generic. If not, it would be a question why snapshot_name_wellformed()
could make sure name not conflict with ID, then reader find the answer
in qcow2...
   I think at least a document is needed about how should implemention
under ./block generate snapshot ID.

On Thu, Jun 13, 2013 at 01:33:29PM +0800, Wenchao Xia wrote:
> 于 2013-6-11 17:14, Stefan Hajnoczi 写道:
> >On Sat, Jun 08, 2013 at 02:58:01PM +0800, Wenchao Xia wrote:
> >>+
> >>+/* 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);
> >>+}
> >
> >Since the caller has to manage id, this function doesn't really abstract
> >anything.  I would keep the snprintf() inline, there's only one caller.
> >
>   Its purpose is move the id rules from one implemention(qcow2) into
> generic. If not, it would be a question why snapshot_name_wellformed()
> could make sure name not conflict with ID, then reader find the answer
> in qcow2...
>   I think at least a document is needed about how should implemention
> under ./block generate snapshot ID.

I see your point.  Maybe keep the function.  I was not sure because the
caller still has the int id and has to increment it.  Therefore it
doesn't fully handle id generation.

Stefan