| Message ID | 20170831035306.29170-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | serial: add serial_chr_nonnull() | expand |
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)".
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 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.
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 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 :)
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,
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(+)