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

[Markus Armbruster]

Philippe Mathieu-Daudé  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 >> > wrote:
>>>
>>>  On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
>>>  > Suggested-by: Peter Maydell >>  >
>>>  > Signed-off-by: Philippe Mathieu-Daudé >>  >
>>>  > ---
>>>  >  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 :)

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

[Philippe Mathieu-Daudé]

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 > wrote:

 On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
 > Suggested-by: Peter Maydell >
 > Signed-off-by: Philippe Mathieu-Daudé >
 > ---
 >  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.

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

[Thomas Huth]

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

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

[Philippe Mathieu-Daudé]

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.

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

[Philippe Mathieu-Daudé]

On 08/31/2017 07:28 AM, Peter Maydell wrote:

On 31 August 2017 at 04:53, Philippe Mathieu-Daudé  wrote:

Suggested-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
  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.

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

[Peter Maydell]

On 31 August 2017 at 04:53, Philippe Mathieu-Daudé  wrote:
> Suggested-by: Peter Maydell 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  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

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

[Thomas Huth]

On 31.08.2017 11:36, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth  > wrote:
> 
> On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
> > Suggested-by: Peter Maydell  >
> > Signed-off-by: Philippe Mathieu-Daudé  >
> > ---
> >  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

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

[Marc-André Lureau]

Hi

On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth  wrote:

> On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
> > Suggested-by: Peter Maydell 
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> >  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

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

[Thomas Huth]

On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
> Suggested-by: Peter Maydell 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  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.