From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:43412)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1ab9Op-0006cy-1Q
	for qemu-devel@nongnu.org; Wed, 02 Mar 2016 11:13:24 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1ab9Oj-0002jX-8k
	for qemu-devel@nongnu.org; Wed, 02 Mar 2016 11:13:18 -0500
References: <fcd74f168507a8ea9a76d1e85eed9f82f75f00c1.1456934833.git.jcody@redhat.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <56D71109.7060009@redhat.com>
Date: Wed, 2 Mar 2016 17:12:57 +0100
MIME-Version: 1.0
In-Reply-To: <fcd74f168507a8ea9a76d1e85eed9f82f75f00c1.1456934833.git.jcody@redhat.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2 1/1] block/sheepdog: fix argument passed
 to qemu_strtoul()
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: Jeff Cody <jcody@redhat.com>, qemu-block@nongnu.org
Cc: kwolf@redhat.com, mitake.hitoshi@lab.ntt.co.jp, qemu-devel@nongnu.org, v.tolstov@selfip.ru, mreitz@redhat.com, namei.unix@gmail.com



On 02/03/2016 17:09, Jeff Cody wrote:
> +    ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id);
> +    if (ret) {
> +        error_setg_errno(errp, -ret, "Invalid snapshot ID: %s",
> +                         snapshot_id ? snapshot_id : "<null>");
> +        return ret;
> +    }
> +
> +    if (snap_id > UINT32_MAX) {
> +        error_setg_errno(errp, EINVAL, "Snapshot ID numeric value %" PRId64
> +                         " exceeds Sheepdog maximum of %" PRId32, snap_id,
> +                          UINT32_MAX);
> +        return -EINVAL;
>      }

I think including the errno produces a worse error message ("Invalid
snapshot ID: foo: Invalid argument" or something like that), and also
the error should be the same for an id of 10^10 (within uint64_t bounds)
or 10^30 (outside the bounds).  So you could just use

    if (ret < 0 || snap_id > UINT32_MAX) {
        error_setg(errp, "Invalid snapshot ID: %s",
                   snapshot_id ? snapshot_id : "<null>");
        return -EINVAL;
    }

Thanks,

Paolo