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

2013-06-13 Thread Stefan Hajnoczi 
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

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

2013-06-12 Thread 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

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

2013-06-11 Thread 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);
}

 +
 +/* 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.