Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: "Thomas Huth" <thuth@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>,
	Eric Blake <eblake@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided
Date: Thu, 31 Aug 2017 12:20:53 -0300	[thread overview]
Message-ID: <800abf63-db3f-78c7-0580-8d86479fdf5a@amsat.org> (raw)
In-Reply-To: <be9ca245-f1af-e78a-aa12-c479e59b2985@redhat.com>

On 08/31/2017 02:19 AM, Thomas Huth wrote:
> On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
[...]>> +Chardev *serial_chr_nonnull(Chardev *chr)
>> +{
>> +    static int serial_id;
>> +    char *label;
>> +
>> +    label = g_strdup_printf("discarding-serial%d", serial_id++);
>> +    chr = qemu_chr_new(label, "null");
> 
> That looks wrong - you're ignoring the input parameter and always open
> the "null" device? Shouldn't there be a "if (chr) return chr;" in front
> of this?

You right. I had this correct in my first patch when this code was 
embedded, I then failed at extracting as another function :/

> 
>> +    assert(chr);
>> +    g_free(label);
>> +
>> +    return chr;
>> +}
> 
>   Thomas
> 
> PS: I think you should also merge the two patches together, they are
> small enough.

Ok.