From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:33070)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <alex.bennee@linaro.org>) id 1Xle8d-0003Cf-Ku
	for qemu-devel@nongnu.org; Tue, 04 Nov 2014 08:27:16 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <alex.bennee@linaro.org>) id 1Xle8Y-0006ZR-RZ
	for qemu-devel@nongnu.org; Tue, 04 Nov 2014 08:27:11 -0500
References: <1415098223-32404-1-git-send-email-zhang.zhanghailiang@huawei.com>
	<1415098223-32404-3-git-send-email-zhang.zhanghailiang@huawei.com>
From: Alex =?utf-8?Q?Benn=C3=A9e?= <alex.bennee@linaro.org>
In-reply-to: <1415098223-32404-3-git-send-email-zhang.zhanghailiang@huawei.com>
Date: Tue, 04 Nov 2014 13:27:06 +0000
Message-ID: <87389zumdx.fsf@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCH v3 2/5] spice-qemu-char: fix parameter
	checks in qemu_chr_parse_* functions
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: zhanghailiang <zhang.zhanghailiang@huawei.com>
Cc: qemu-devel@nongnu.org, qemu-trivial@nongnu.org, mjt@tls.msk.ru, peter.huangpeng@huawei.com, armbru@redhat.com, kraxel@redhat.com


zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> For functions qemu_chr_parse_spice_vmc and qemu_chr_parse_spice_port,
> we should also check if parameter name is empty, and it will help finding
> the wrong configure, such as
> 'qemu-system-x86_64 -chardev spiceport(or spiceport),id=id,name='
>
> After check the parameter in parse function, we can remove the
> superfluous parameter checks in qemu_chr_open_spice_*
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  spice-qemu-char.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> index 8106e06..d3c1f5c 100644
> --- a/spice-qemu-char.c
> +++ b/spice-qemu-char.c
> @@ -290,11 +290,6 @@ CharDriverState *qemu_chr_open_spice_vmc(const char *type)
>  {
>      const char **psubtype = spice_server_char_device_recognized_subtypes();
>  
> -    if (type == NULL) {
> -        fprintf(stderr, "spice-qemu-char: missing name parameter\n");
> -        print_allowed_subtypes();
> -        return NULL;
> -    }

As with patch 1 I'd at least assert the contract. But this also drops a
helpful info dump on a missing parameter.

>      for (; *psubtype != NULL; ++psubtype) {
>          if (strcmp(type, *psubtype) == 0) {
>              break;
> @@ -315,11 +310,6 @@ CharDriverState *qemu_chr_open_spice_port(const char *name)
>      CharDriverState *chr;
>      SpiceCharDriver *s;
>  
> -    if (name == NULL) {
> -        fprintf(stderr, "spice-qemu-char: missing name parameter\n");
> -        return NULL;
> -    }
> -
>      chr = chr_open("port", spice_port_set_fe_open);
>      s = chr->opaque;
>      s->sin.portname = g_strdup(name);
> @@ -345,7 +335,7 @@ static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
>  {
>      const char *name = qemu_opt_get(opts, "name");
>  
> -    if (name == NULL) {
> +    if (name == NULL || !name[0]) {
>          error_setg(errp, "chardev: spice channel: no name given");
>          return;
>      }
> @@ -358,7 +348,7 @@ static void qemu_chr_parse_spice_port(QemuOpts *opts, ChardevBackend *backend,
>  {
>      const char *name = qemu_opt_get(opts, "name");
>  
> -    if (name == NULL) {
> +    if (name == NULL || !name[0]) {
>          error_setg(errp, "chardev: spice port: no name given");
>          return;
>      }

Same comments as for 1/5

-- 
Alex Bennée