Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/char/serial.h | 1 +
hw/char/serial.c | 13 +++++++++++++
2 files changed, 14 insertions(+)
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index c4daf11a14..96bccb39e1 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
hwaddr base, int it_shift,
qemu_irq irq, int baudbase,
Chardev *chr, enum device_endian end);
+Chardev *serial_chr_nonnull(Chardev *chr);
/* serial-isa.c */
#define TYPE_ISA_SERIAL "isa-serial"
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 9aec6c60d8..7a100db107 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1025,6 +1025,19 @@ static const MemoryRegionOps serial_mm_ops[3] = {
},
};
+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");
+ assert(chr);
+ g_free(label);
+
+ return chr;
+}
+
SerialState *serial_mm_init(MemoryRegion *address_space,
hwaddr base, int it_shift,
qemu_irq irq, int baudbase,
--
2.14.1
On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/hw/char/serial.h | 1 +
> hw/char/serial.c | 13 +++++++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index c4daf11a14..96bccb39e1 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
> hwaddr base, int it_shift,
> qemu_irq irq, int baudbase,
> Chardev *chr, enum device_endian end);
> +Chardev *serial_chr_nonnull(Chardev *chr);
Why do you need the prototype? Please make the function static if
possible (otherwise please add some rationale in the patch description).
> /* serial-isa.c */
> #define TYPE_ISA_SERIAL "isa-serial"
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 9aec6c60d8..7a100db107 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -1025,6 +1025,19 @@ static const MemoryRegionOps serial_mm_ops[3] = {
> },
> };
>
> +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?
> + assert(chr);
> + g_free(label);
> +
> + return chr;
> +}
Thomas
PS: I think you should also merge the two patches together, they are
small enough.
Hi
On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth <thuth@redhat.com> wrote:
> On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> > include/hw/char/serial.h | 1 +
> > hw/char/serial.c | 13 +++++++++++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> > index c4daf11a14..96bccb39e1 100644
> > --- a/include/hw/char/serial.h
> > +++ b/include/hw/char/serial.h
> > @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion
> *address_space,
> > hwaddr base, int it_shift,
> > qemu_irq irq, int baudbase,
> > Chardev *chr, enum device_endian end);
> > +Chardev *serial_chr_nonnull(Chardev *chr);
>
> Why do you need the prototype? Please make the function static if
> possible (otherwise please add some rationale in the patch description).
>
It's being used from other units in following patches
> > /* serial-isa.c */
> > #define TYPE_ISA_SERIAL "isa-serial"
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index 9aec6c60d8..7a100db107 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -1025,6 +1025,19 @@ static const MemoryRegionOps serial_mm_ops[3] = {
> > },
> > };
> >
> > +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?
>
I think its missing too.
The function name and usage isn't obvious. I would rather see the caller
use "chr ?: serial_chr_null()" rather than "serial_chr_nonnull(chr)".
--
Marc-André Lureau
On 31.08.2017 11:36, Marc-André Lureau wrote: > Hi > > On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth <thuth@redhat.com > <mailto:thuth@redhat.com>> wrote: > > On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote: > > Suggested-by: Peter Maydell <peter.maydell@linaro.org > <mailto:peter.maydell@linaro.org>> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org > <mailto:f4bug@amsat.org>> > > --- > > include/hw/char/serial.h | 1 + > > hw/char/serial.c | 13 +++++++++++++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h > > index c4daf11a14..96bccb39e1 100644 > > --- a/include/hw/char/serial.h > > +++ b/include/hw/char/serial.h > > @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion > *address_space, > > hwaddr base, int it_shift, > > qemu_irq irq, int baudbase, > > Chardev *chr, enum device_endian end); > > +Chardev *serial_chr_nonnull(Chardev *chr); > > Why do you need the prototype? Please make the function static if > possible (otherwise please add some rationale in the patch description). > > It's being used from other units in following patches Ah, well, right. I was only on CC: in the first two patches, so I missed the other ones at the first glance. So never mind my comment, the prototype is fine here. Thomas
On 08/31/2017 06:43 AM, Thomas Huth wrote: > On 31.08.2017 11:36, Marc-André Lureau wrote: >> Hi >> >> On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth <thuth@redhat.com >> <mailto:thuth@redhat.com>> wrote: >> >> On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote: >> > Suggested-by: Peter Maydell <peter.maydell@linaro.org >> <mailto:peter.maydell@linaro.org>> >> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org >> <mailto:f4bug@amsat.org>> >> > --- >> > include/hw/char/serial.h | 1 + >> > hw/char/serial.c | 13 +++++++++++++ >> > 2 files changed, 14 insertions(+) >> > >> > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h >> > index c4daf11a14..96bccb39e1 100644 >> > --- a/include/hw/char/serial.h >> > +++ b/include/hw/char/serial.h >> > @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion >> *address_space, >> > hwaddr base, int it_shift, >> > qemu_irq irq, int baudbase, >> > Chardev *chr, enum device_endian end); >> > +Chardev *serial_chr_nonnull(Chardev *chr); >> >> Why do you need the prototype? Please make the function static if >> possible (otherwise please add some rationale in the patch description). >> >> It's being used from other units in following patches > > Ah, well, right. I was only on CC: in the first two patches, so I missed > the other ones at the first glance. So never mind my comment, the > prototype is fine here. Is it better/easier to use the same list for the cover and all the patches? I try to shorten the it to help overloaded reviewer to focus on the patches I think they can help. But this case show it's not that useful.
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 08/31/2017 06:43 AM, Thomas Huth wrote: >> On 31.08.2017 11:36, Marc-André Lureau wrote: >>> Hi >>> >>> On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth <thuth@redhat.com >>> <mailto:thuth@redhat.com>> wrote: >>> >>> On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote: >>> > Suggested-by: Peter Maydell <peter.maydell@linaro.org >>> <mailto:peter.maydell@linaro.org>> >>> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org >>> <mailto:f4bug@amsat.org>> >>> > --- >>> > include/hw/char/serial.h | 1 + >>> > hw/char/serial.c | 13 +++++++++++++ >>> > 2 files changed, 14 insertions(+) >>> > >>> > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h >>> > index c4daf11a14..96bccb39e1 100644 >>> > --- a/include/hw/char/serial.h >>> > +++ b/include/hw/char/serial.h >>> > @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion >>> *address_space, >>> > hwaddr base, int it_shift, >>> > qemu_irq irq, int baudbase, >>> > Chardev *chr, enum device_endian end); >>> > +Chardev *serial_chr_nonnull(Chardev *chr); >>> >>> Why do you need the prototype? Please make the function static if >>> possible (otherwise please add some rationale in the patch description). >>> >>> It's being used from other units in following patches >> >> Ah, well, right. I was only on CC: in the first two patches, so I missed >> the other ones at the first glance. So never mind my comment, the >> prototype is fine here. > > Is it better/easier to use the same list for the cover and all the patches? > I try to shorten the it to help overloaded reviewer to focus on the > patches I think they can help. But this case show it's not that > useful. Clear an unequivocal answer: it depends :)
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.
On 31.08.2017 17:20, Philippe Mathieu-Daudé wrote:
> 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.
Well, I wrote that comment about merging the two patches together when I
was thinking that your series consists of only two patches (since I've
only been CC:-ed on the first two patches). So please simply ignore that
suggestion :-)
Thomas
On 31 August 2017 at 04:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/hw/char/serial.h | 1 + > hw/char/serial.c | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h > index c4daf11a14..96bccb39e1 100644 > --- a/include/hw/char/serial.h > +++ b/include/hw/char/serial.h > @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space, > hwaddr base, int it_shift, > qemu_irq irq, int baudbase, > Chardev *chr, enum device_endian end); > +Chardev *serial_chr_nonnull(Chardev *chr); For new globally-visible function declarations, can we have a doc-comment form comment that documents the function, please? thanks -- PMM
On 08/31/2017 07:28 AM, Peter Maydell wrote: > On 31 August 2017 at 04:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> include/hw/char/serial.h | 1 + >> hw/char/serial.c | 13 +++++++++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h >> index c4daf11a14..96bccb39e1 100644 >> --- a/include/hw/char/serial.h >> +++ b/include/hw/char/serial.h >> @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space, >> hwaddr base, int it_shift, >> qemu_irq irq, int baudbase, >> Chardev *chr, enum device_endian end); >> +Chardev *serial_chr_nonnull(Chardev *chr); > > For new globally-visible function declarations, can we > have a doc-comment form comment that documents the > function, please? I was afraid someone asked that, but since this file has no other comment I tried :p Good to know for future contributions, someone fluent with English should add this to CODING_STYLE.
© 2016 - 2025 Red Hat, Inc.