[3/3] ipoctal232: Convert to use chardev properties directly

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Cc: Alberto Garcia <agarcia@igalia.com>
---
 hw/ipoctal232.c | 43 ++++++++++++++-----------------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

Hans de Goede <hdegoede@redhat.com> writes:

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Cc: Alberto Garcia <agarcia@igalia.com>

I don't think this is a show stopper, but this is a compatibility
breaker, no?

We should be more up front about that and include release notes as
appropriate.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

> ---
>  hw/ipoctal232.c | 43 ++++++++++++++-----------------------------
>  1 file changed, 14 insertions(+), 29 deletions(-)
>
> diff --git a/hw/ipoctal232.c b/hw/ipoctal232.c
> index 345efae..685fee2 100644
> --- a/hw/ipoctal232.c
> +++ b/hw/ipoctal232.c
> @@ -93,7 +93,6 @@ typedef struct SCC2698Block SCC2698Block;
>  struct SCC2698Channel {
>      IPOctalState *ipoctal;
>      CharDriverState *dev;
> -    char *devpath;
>      bool rx_enabled;
>      uint8_t mr[2];
>      uint8_t mr_idx;
> @@ -545,26 +544,12 @@ static int ipoctal_init(IPackDevice *ip)
>          ch->ipoctal = s;
>  
>          /* Redirect IP-Octal channels to host character devices */
> -        if (ch->devpath) {
> -            const char chr_name[] = "ipoctal";
> -            char label[ARRAY_SIZE(chr_name) + 2];
> -            static int index;
> -
> -            snprintf(label, sizeof(label), "%s%d", chr_name, index);
> -
> -            ch->dev = qemu_chr_new(label, ch->devpath, NULL);
> -
> -            if (ch->dev) {
> -                index++;
> -                qemu_chr_fe_claim_no_fail(ch->dev);
> -                qemu_chr_add_handlers(ch->dev, hostdev_can_receive,
> -                                      hostdev_receive, hostdev_event, ch);
> -                DPRINTF("Redirecting channel %u to %s (%s)\n",
> -                        i, ch->devpath, label);
> -            } else {
> -                DPRINTF("Could not redirect channel %u to %s\n",
> -                        i, ch->devpath);
> -            }
> +        if (ch->dev) {
> +            qemu_chr_add_handlers(ch->dev, hostdev_can_receive,
> +                                  hostdev_receive, hostdev_event, ch);
> +            DPRINTF("Redirecting channel %u to %s\n", i, ch->dev->label);
> +        } else {
> +            DPRINTF("Could not redirect channel %u, no chardev set\n", i);
>          }
>      }
>  
> @@ -572,14 +557,14 @@ static int ipoctal_init(IPackDevice *ip)
>  }
>  
>  static Property ipoctal_properties[] = {
> -    DEFINE_PROP_STRING("serial0", IPOctalState, ch[0].devpath),
> -    DEFINE_PROP_STRING("serial1", IPOctalState, ch[1].devpath),
> -    DEFINE_PROP_STRING("serial2", IPOctalState, ch[2].devpath),
> -    DEFINE_PROP_STRING("serial3", IPOctalState, ch[3].devpath),
> -    DEFINE_PROP_STRING("serial4", IPOctalState, ch[4].devpath),
> -    DEFINE_PROP_STRING("serial5", IPOctalState, ch[5].devpath),
> -    DEFINE_PROP_STRING("serial6", IPOctalState, ch[6].devpath),
> -    DEFINE_PROP_STRING("serial7", IPOctalState, ch[7].devpath),
> +    DEFINE_PROP_CHR("chardev0", IPOctalState, ch[0].dev),
> +    DEFINE_PROP_CHR("chardev1", IPOctalState, ch[1].dev),
> +    DEFINE_PROP_CHR("chardev2", IPOctalState, ch[2].dev),
> +    DEFINE_PROP_CHR("chardev3", IPOctalState, ch[3].dev),
> +    DEFINE_PROP_CHR("chardev4", IPOctalState, ch[4].dev),
> +    DEFINE_PROP_CHR("chardev5", IPOctalState, ch[5].dev),
> +    DEFINE_PROP_CHR("chardev6", IPOctalState, ch[6].dev),
> +    DEFINE_PROP_CHR("chardev7", IPOctalState, ch[7].dev),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -- 
> 1.8.1.4

> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > Cc: Alberto Garcia <agarcia@igalia.com>
> 
> I don't think this is a show stopper, but this is a compatibility
> breaker, no?
> 
> We should be more up front about that and include release notes as
> appropriate.

Yes on all counts.  I can work on the release notes.

Paolo

> Given that you requested this change, it indeed probably is best if
> you did this :)  Feel free to squash this into the patch if that is
> preferred (and to make yourself the author of the patch in that
> case).
> 
> So how are we going to go about upstreaming these? Anthony will you
> pick 1 + 2 up directly, and then 3 once the release notes are there,
> or ... ?

The release notes are on the wiki.

Paolo

Hi,

On 03/28/2013 07:18 AM, Paolo Bonzini wrote:
>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Cc: Alberto Garcia <agarcia@igalia.com>
>>
>> I don't think this is a show stopper, but this is a compatibility
>> breaker, no?
>>
>> We should be more up front about that and include release notes as
>> appropriate.
>
> Yes on all counts.  I can work on the release notes.

Given that you requested this change, it indeed probably is best if
you did this :)  Feel free to squash this into the patch if that is
preferred (and to make yourself the author of the patch in that case).

So how are we going to go about upstreaming these? Anthony will you
pick 1 + 2 up directly, and then 3 once the release notes are there,
or ... ?

Regards,

Hans

On Wed, Mar 27, 2013 at 08:29:41PM +0100, Hans de Goede wrote:

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Cc: Alberto Garcia <agarcia@igalia.com>

Sorry for the delay, just came back from holidays :)

I've just tested it and the change looks fine to me.

Signed-off-by: Alberto Garcia <agarcia@igalia.com>

Berto