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

login
register
mail settings
Submitter Hans de Goede
Date March 27, 2013, 7:29 p.m.
Message ID <1364412581-3672-4-git-send-email-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/231814/
State New
Headers show

Comments

Hans de Goede - March 27, 2013, 7:29 p.m.
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(-)
Anthony Liguori - March 27, 2013, 9:18 p.m.
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
Paolo Bonzini - March 28, 2013, 6:18 a.m.
> > 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
Paolo Bonzini - March 28, 2013, 11:39 a.m.
> 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
Hans de Goede - March 28, 2013, 11:42 a.m.
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
Alberto Garcia - April 1, 2013, 9:38 p.m.
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

Patch

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(),
 };