From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:51125)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1UmKeZ-00080F-6F
	for qemu-devel@nongnu.org; Tue, 11 Jun 2013 05:14:12 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1UmKeX-00014e-CW
	for qemu-devel@nongnu.org; Tue, 11 Jun 2013 05:14:11 -0400
Received: from mail-ea0-x233.google.com ([2a00:1450:4013:c01::233]:53893)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1UmKeX-00014E-5A
	for qemu-devel@nongnu.org; Tue, 11 Jun 2013 05:14:09 -0400
Received: by mail-ea0-f179.google.com with SMTP id b15so4840489eae.38
	for <qemu-devel@nongnu.org>; Tue, 11 Jun 2013 02:14:08 -0700 (PDT)
Date: Tue, 11 Jun 2013 11:14:05 +0200
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20130611091405.GF18312@stefanha-thinkpad.redhat.com>
References: <1370674687-13849-1-git-send-email-xiawenc@linux.vnet.ibm.com>
	<1370674687-13849-6-git-send-email-xiawenc@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1370674687-13849-6-git-send-email-xiawenc@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH 05/11] snapshot: add paired functions for
 internal snapshot id and name
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Cc: kwolf@redhat.com, phrdina@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com

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.