Re: [Qemu-devel] [PATCH 05/11] snapshot: add paired functions for internal snapshot id and name

[Wenchao Xia]

于 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.

--
Best Regards

Wenchao Xia