[Qemu-devel] [PATCH] Change return type of functions that are named *_exit or *_exitfn in hw/ from int to void.

[Qemu-devel] [PATCH] Change return type of functions that are named *_exit or *_exitfn in hw/ from int to void.

[Nutan Shinde]

Functions that are named  *_exit or *_exitfn in hw/, all return 0. Changed return type of these methods from int to void.
Also, removed check where values were returned from these methods.

Signed-off-by: Nutan Shinde <nutanshinde1992@gmail.com>
---
 hw/audio/hda-codec.c          | 3 +--
 hw/audio/intel-hda.c          | 3 +--
 hw/char/sclpconsole-lm.c      | 3 +--
 hw/char/sclpconsole.c         | 3 +--
 hw/s390x/virtio-ccw.c         | 7 ++-----
 hw/usb/ccid-card-emulated.c   | 3 +--
 hw/usb/dev-smartcard-reader.c | 8 ++------
 7 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index 52d4640..5402cd1 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -520,7 +520,7 @@ static int hda_audio_init(HDACodecDevice *hda, const struct desc_codec *desc)
     return 0;
 }
 
-static int hda_audio_exit(HDACodecDevice *hda)
+static void hda_audio_exit(HDACodecDevice *hda)
 {
     HDAAudioState *a = HDA_AUDIO(hda);
     HDAAudioStream *st;
@@ -539,7 +539,6 @@ static int hda_audio_exit(HDACodecDevice *hda)
         }
     }
     AUD_remove_card(&a->card);
-    return 0;
 }
 
 static int hda_audio_post_load(void *opaque, int version)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index d372d4a..404cfcf 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -66,7 +66,7 @@ static int hda_codec_dev_init(DeviceState *qdev)
     return cdc->init(dev);
 }
 
-static int hda_codec_dev_exit(DeviceState *qdev)
+static void hda_codec_dev_exit(DeviceState *qdev)
 {
     HDACodecDevice *dev = DO_UPCAST(HDACodecDevice, qdev, qdev);
     HDACodecDeviceClass *cdc = HDA_CODEC_DEVICE_GET_CLASS(dev);
@@ -74,7 +74,6 @@ static int hda_codec_dev_exit(DeviceState *qdev)
     if (cdc->exit) {
         cdc->exit(dev);
     }
-    return 0;
 }
 
 HDACodecDevice *hda_codec_find(HDACodecBus *bus, uint32_t cad)
diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
index 7d4ff81..d2fe30a 100644
--- a/hw/char/sclpconsole-lm.c
+++ b/hw/char/sclpconsole-lm.c
@@ -328,9 +328,8 @@ static int console_init(SCLPEvent *event)
     return 0;
 }
 
-static int console_exit(SCLPEvent *event)
+static void console_exit(SCLPEvent *event)
 {
-    return 0;
 }
 
 static void console_reset(DeviceState *dev)
diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
index 45997ff..da7b503 100644
--- a/hw/char/sclpconsole.c
+++ b/hw/char/sclpconsole.c
@@ -242,9 +242,8 @@ static void console_reset(DeviceState *dev)
    scon->notify = false;
 }
 
-static int console_exit(SCLPEvent *event)
+static void console_exit(SCLPEvent *event)
 {
-    return 0;
 }
 
 static Property console_properties[] = {
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index cb887ba..74e989b 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -877,7 +877,7 @@ out_err:
     g_free(sch);
 }
 
-static int virtio_ccw_exit(VirtioCcwDevice *dev)
+static void virtio_ccw_exit(VirtioCcwDevice *dev)
 {
     SubchDev *sch = dev->sch;
 
@@ -889,7 +889,6 @@ static int virtio_ccw_exit(VirtioCcwDevice *dev)
         release_indicator(&dev->routes.adapter, dev->indicators);
         dev->indicators = NULL;
     }
-    return 0;
 }
 
 static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
@@ -1734,12 +1733,10 @@ static void virtio_ccw_busdev_realize(DeviceState *dev, Error **errp)
     virtio_ccw_device_realize(_dev, errp);
 }
 
-static int virtio_ccw_busdev_exit(DeviceState *dev)
+static void virtio_ccw_busdev_exit(DeviceState *dev)
 {
     VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
     VirtIOCCWDeviceClass *_info = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
-
-    return _info->exit(_dev);
 }
 
 static void virtio_ccw_busdev_unplug(HotplugHandler *hotplug_dev,
diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 9ddd5ad..9962786 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -547,7 +547,7 @@ static int emulated_initfn(CCIDCardState *base)
     return 0;
 }
 
-static int emulated_exitfn(CCIDCardState *base)
+static void emulated_exitfn(CCIDCardState *base)
 {
     EmulatedState *card = EMULATED_CCID_CARD(base);
     VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL);
@@ -564,7 +564,6 @@ static int emulated_exitfn(CCIDCardState *base)
     qemu_mutex_destroy(&card->handle_apdu_mutex);
     qemu_mutex_destroy(&card->vreader_mutex);
     qemu_mutex_destroy(&card->event_list_mutex);
-    return 0;
 }
 
 static Property emulated_card_properties[] = {
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 96a1a13..6cc18b1 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -507,14 +507,13 @@ static void ccid_card_apdu_from_guest(CCIDCardState *card,
     }
 }
 
-static int ccid_card_exitfn(CCIDCardState *card)
+static void ccid_card_exitfn(CCIDCardState *card)
 {
     CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
 
     if (cc->exitfn) {
         return cc->exitfn(card);
     }
-    return 0;
 }
 
 static int ccid_card_initfn(CCIDCardState *card)
@@ -1276,9 +1275,8 @@ void ccid_card_card_inserted(CCIDCardState *card)
     ccid_on_slot_change(s, true);
 }
 
-static int ccid_card_exit(DeviceState *qdev)
+static void ccid_card_exit(DeviceState *qdev)
 {
-    int ret = 0;
     CCIDCardState *card = CCID_CARD(qdev);
     USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
@@ -1286,9 +1284,7 @@ static int ccid_card_exit(DeviceState *qdev)
     if (ccid_card_inserted(s)) {
         ccid_card_card_removed(card);
     }
-    ret = ccid_card_exitfn(card);
     s->card = NULL;
-    return ret;
 }
 
 static int ccid_card_init(DeviceState *qdev)
-- 
1.9.1

Re: [Qemu-devel] [PATCH] Change return type of functions that are named *_exit or *_exitfn in hw/ from int to void.

[Peter Maydell]

On 24 March 2016 at 17:18, Nutan Shinde <nutanshinde1992@gmail.com> wrote:

Hi; thanks for this patch; I have some comments below.

> Functions that are named  *_exit or *_exitfn in hw/, all return 0. Changed return type of these methods from int to void.
> Also, removed check where values were returned from these methods.

In general, commit messages should describe the "why" of a
change, not just the "how". They should also be line wrapped
at about 70 characters.


> Signed-off-by: Nutan Shinde <nutanshinde1992@gmail.com>
> ---
>  hw/audio/hda-codec.c          | 3 +--
>  hw/audio/intel-hda.c          | 3 +--
>  hw/char/sclpconsole-lm.c      | 3 +--
>  hw/char/sclpconsole.c         | 3 +--
>  hw/s390x/virtio-ccw.c         | 7 ++-----
>  hw/usb/ccid-card-emulated.c   | 3 +--
>  hw/usb/dev-smartcard-reader.c | 8 ++------
>  7 files changed, 9 insertions(+), 21 deletions(-)

It's usually not a good idea to have one patch which touches
multiple completely different devices or subsystems.

>
> diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
> index 52d4640..5402cd1 100644
> --- a/hw/audio/hda-codec.c
> +++ b/hw/audio/hda-codec.c
> @@ -520,7 +520,7 @@ static int hda_audio_init(HDACodecDevice *hda, const struct desc_codec *desc)
>      return 0;
>  }
>
> -static int hda_audio_exit(HDACodecDevice *hda)
> +static void hda_audio_exit(HDACodecDevice *hda)
>  {
>      HDAAudioState *a = HDA_AUDIO(hda);
>      HDAAudioStream *st;
> @@ -539,7 +539,6 @@ static int hda_audio_exit(HDACodecDevice *hda)
>          }
>      }
>      AUD_remove_card(&a->card);
> -    return 0;
>  }
>

This function is used to initialize the 'exit' function pointer
in the HDACodecDeviceClass structure, but you haven't changed the
signature of that function pointer to match the change in the
function type here. (I'm surprised this compiles.)

>  static int hda_audio_post_load(void *opaque, int version)
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index d372d4a..404cfcf 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -66,7 +66,7 @@ static int hda_codec_dev_init(DeviceState *qdev)
>      return cdc->init(dev);
>  }
>
> -static int hda_codec_dev_exit(DeviceState *qdev)
> +static void hda_codec_dev_exit(DeviceState *qdev)
>  {
>      HDACodecDevice *dev = DO_UPCAST(HDACodecDevice, qdev, qdev);
>      HDACodecDeviceClass *cdc = HDA_CODEC_DEVICE_GET_CLASS(dev);
> @@ -74,7 +74,6 @@ static int hda_codec_dev_exit(DeviceState *qdev)
>      if (cdc->exit) {
>          cdc->exit(dev);
>      }
> -    return 0;
>  }
>
>  HDACodecDevice *hda_codec_find(HDACodecBus *bus, uint32_t cad)

This function is implementing the DeviceClass exit method, so you
cannot just change its return type. Changing the method type would
be OK I think (though probably we ought to convert to realize and
unrealize in the longer term, so I'm not sure how much it's worth
changing the type of the exit method.)

> diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> index 7d4ff81..d2fe30a 100644
> --- a/hw/char/sclpconsole-lm.c
> +++ b/hw/char/sclpconsole-lm.c
> @@ -328,9 +328,8 @@ static int console_init(SCLPEvent *event)
>      return 0;
>  }
>
> -static int console_exit(SCLPEvent *event)
> +static void console_exit(SCLPEvent *event)
>  {
> -    return 0;
>  }
>
>  static void console_reset(DeviceState *dev)
> diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
> index 45997ff..da7b503 100644
> --- a/hw/char/sclpconsole.c
> +++ b/hw/char/sclpconsole.c
> @@ -242,9 +242,8 @@ static void console_reset(DeviceState *dev)
>     scon->notify = false;
>  }
>
> -static int console_exit(SCLPEvent *event)
> +static void console_exit(SCLPEvent *event)
>  {
> -    return 0;
>  }
>
>  static Property console_properties[] = {

These two functions are implementing the SCLPEventClass
exit method, so you can't just change these without changing
the method's type as well. (I don't know if that change is
a good idea, the s390 maintainers would have to say.)

> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index cb887ba..74e989b 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -877,7 +877,7 @@ out_err:
>      g_free(sch);
>  }
>
> -static int virtio_ccw_exit(VirtioCcwDevice *dev)
> +static void virtio_ccw_exit(VirtioCcwDevice *dev)
>  {
>      SubchDev *sch = dev->sch;
>
> @@ -889,7 +889,6 @@ static int virtio_ccw_exit(VirtioCcwDevice *dev)
>          release_indicator(&dev->routes.adapter, dev->indicators);
>          dev->indicators = NULL;
>      }
> -    return 0;
>  }
>
>  static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
> @@ -1734,12 +1733,10 @@ static void virtio_ccw_busdev_realize(DeviceState *dev, Error **errp)
>      virtio_ccw_device_realize(_dev, errp);
>  }
>
> -static int virtio_ccw_busdev_exit(DeviceState *dev)
> +static void virtio_ccw_busdev_exit(DeviceState *dev)
>  {
>      VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
>      VirtIOCCWDeviceClass *_info = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
> -
> -    return _info->exit(_dev);
>  }

These also are changing the implementation of a method but not its type.

>
>  static void virtio_ccw_busdev_unplug(HotplugHandler *hotplug_dev,
> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> index 9ddd5ad..9962786 100644
> --- a/hw/usb/ccid-card-emulated.c
> +++ b/hw/usb/ccid-card-emulated.c
> @@ -547,7 +547,7 @@ static int emulated_initfn(CCIDCardState *base)
>      return 0;
>  }
>
> -static int emulated_exitfn(CCIDCardState *base)
> +static void emulated_exitfn(CCIDCardState *base)
>  {
>      EmulatedState *card = EMULATED_CCID_CARD(base);
>      VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL);
> @@ -564,7 +564,6 @@ static int emulated_exitfn(CCIDCardState *base)
>      qemu_mutex_destroy(&card->handle_apdu_mutex);
>      qemu_mutex_destroy(&card->vreader_mutex);
>      qemu_mutex_destroy(&card->event_list_mutex);
> -    return 0;
>  }
>

as is this.

>  static Property emulated_card_properties[] = {
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 96a1a13..6cc18b1 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -507,14 +507,13 @@ static void ccid_card_apdu_from_guest(CCIDCardState *card,
>      }
>  }
>
> -static int ccid_card_exitfn(CCIDCardState *card)
> +static void ccid_card_exitfn(CCIDCardState *card)
>  {
>      CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
>
>      if (cc->exitfn) {
>          return cc->exitfn(card);
>      }
> -    return 0;
>  }
>

This change has made the function's return type 'void' but
there is still a line in it which is returning a value.

Also, since the CCIDCardClass exitfn method returns a
value, this function should too (its purpose is just to
be a wrapper around the method call).

>  static int ccid_card_initfn(CCIDCardState *card)
> @@ -1276,9 +1275,8 @@ void ccid_card_card_inserted(CCIDCardState *card)
>      ccid_on_slot_change(s, true);
>  }
>
> -static int ccid_card_exit(DeviceState *qdev)
> +static void ccid_card_exit(DeviceState *qdev)
>  {
> -    int ret = 0;
>      CCIDCardState *card = CCID_CARD(qdev);
>      USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
>      USBCCIDState *s = USB_CCID_DEV(dev);
> @@ -1286,9 +1284,7 @@ static int ccid_card_exit(DeviceState *qdev)
>      if (ccid_card_inserted(s)) {
>          ccid_card_card_removed(card);
>      }
> -    ret = ccid_card_exitfn(card);
>      s->card = NULL;
> -    return ret;

This change has lost the call to ccid_card_exitfn().

>  }
>
>  static int ccid_card_init(DeviceState *qdev)
> --
> 1.9.1
>
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Change return type of functions that are named *_exit or *_exitfn in hw/ from int to void.

[Nutan Shinde]

[-- Attachment #1: Type: text/plain, Size: 8448 bytes --]

Hi Peter,

I don't know why this change is required. I picked up this task from
BitSizedTask. Can you please, tell me the purpose of this task?

Also, taking your comments into consideration, I shall re-send the patch.

Regards,
Nutan.

On Thu, Mar 24, 2016 at 11:49 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 24 March 2016 at 17:18, Nutan Shinde <nutanshinde1992@gmail.com> wrote:
>
> Hi; thanks for this patch; I have some comments below.
>
> > Functions that are named  *_exit or *_exitfn in hw/, all return 0.
> Changed return type of these methods from int to void.
> > Also, removed check where values were returned from these methods.
>
> In general, commit messages should describe the "why" of a
> change, not just the "how". They should also be line wrapped
> at about 70 characters.
>
>
> > Signed-off-by: Nutan Shinde <nutanshinde1992@gmail.com>
> > ---
> >  hw/audio/hda-codec.c          | 3 +--
> >  hw/audio/intel-hda.c          | 3 +--
> >  hw/char/sclpconsole-lm.c      | 3 +--
> >  hw/char/sclpconsole.c         | 3 +--
> >  hw/s390x/virtio-ccw.c         | 7 ++-----
> >  hw/usb/ccid-card-emulated.c   | 3 +--
> >  hw/usb/dev-smartcard-reader.c | 8 ++------
> >  7 files changed, 9 insertions(+), 21 deletions(-)
>
> It's usually not a good idea to have one patch which touches
> multiple completely different devices or subsystems.
>
> >
> > diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
> > index 52d4640..5402cd1 100644
> > --- a/hw/audio/hda-codec.c
> > +++ b/hw/audio/hda-codec.c
> > @@ -520,7 +520,7 @@ static int hda_audio_init(HDACodecDevice *hda, const
> struct desc_codec *desc)
> >      return 0;
> >  }
> >
> > -static int hda_audio_exit(HDACodecDevice *hda)
> > +static void hda_audio_exit(HDACodecDevice *hda)
> >  {
> >      HDAAudioState *a = HDA_AUDIO(hda);
> >      HDAAudioStream *st;
> > @@ -539,7 +539,6 @@ static int hda_audio_exit(HDACodecDevice *hda)
> >          }
> >      }
> >      AUD_remove_card(&a->card);
> > -    return 0;
> >  }
> >
>
> This function is used to initialize the 'exit' function pointer
> in the HDACodecDeviceClass structure, but you haven't changed the
> signature of that function pointer to match the change in the
> function type here. (I'm surprised this compiles.)
>
> >  static int hda_audio_post_load(void *opaque, int version)
> > diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> > index d372d4a..404cfcf 100644
> > --- a/hw/audio/intel-hda.c
> > +++ b/hw/audio/intel-hda.c
> > @@ -66,7 +66,7 @@ static int hda_codec_dev_init(DeviceState *qdev)
> >      return cdc->init(dev);
> >  }
> >
> > -static int hda_codec_dev_exit(DeviceState *qdev)
> > +static void hda_codec_dev_exit(DeviceState *qdev)
> >  {
> >      HDACodecDevice *dev = DO_UPCAST(HDACodecDevice, qdev, qdev);
> >      HDACodecDeviceClass *cdc = HDA_CODEC_DEVICE_GET_CLASS(dev);
> > @@ -74,7 +74,6 @@ static int hda_codec_dev_exit(DeviceState *qdev)
> >      if (cdc->exit) {
> >          cdc->exit(dev);
> >      }
> > -    return 0;
> >  }
> >
> >  HDACodecDevice *hda_codec_find(HDACodecBus *bus, uint32_t cad)
>
> This function is implementing the DeviceClass exit method, so you
> cannot just change its return type. Changing the method type would
> be OK I think (though probably we ought to convert to realize and
> unrealize in the longer term, so I'm not sure how much it's worth
> changing the type of the exit method.)
>
> > diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> > index 7d4ff81..d2fe30a 100644
> > --- a/hw/char/sclpconsole-lm.c
> > +++ b/hw/char/sclpconsole-lm.c
> > @@ -328,9 +328,8 @@ static int console_init(SCLPEvent *event)
> >      return 0;
> >  }
> >
> > -static int console_exit(SCLPEvent *event)
> > +static void console_exit(SCLPEvent *event)
> >  {
> > -    return 0;
> >  }
> >
> >  static void console_reset(DeviceState *dev)
> > diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
> > index 45997ff..da7b503 100644
> > --- a/hw/char/sclpconsole.c
> > +++ b/hw/char/sclpconsole.c
> > @@ -242,9 +242,8 @@ static void console_reset(DeviceState *dev)
> >     scon->notify = false;
> >  }
> >
> > -static int console_exit(SCLPEvent *event)
> > +static void console_exit(SCLPEvent *event)
> >  {
> > -    return 0;
> >  }
> >
> >  static Property console_properties[] = {
>
> These two functions are implementing the SCLPEventClass
> exit method, so you can't just change these without changing
> the method's type as well. (I don't know if that change is
> a good idea, the s390 maintainers would have to say.)
>
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index cb887ba..74e989b 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -877,7 +877,7 @@ out_err:
> >      g_free(sch);
> >  }
> >
> > -static int virtio_ccw_exit(VirtioCcwDevice *dev)
> > +static void virtio_ccw_exit(VirtioCcwDevice *dev)
> >  {
> >      SubchDev *sch = dev->sch;
> >
> > @@ -889,7 +889,6 @@ static int virtio_ccw_exit(VirtioCcwDevice *dev)
> >          release_indicator(&dev->routes.adapter, dev->indicators);
> >          dev->indicators = NULL;
> >      }
> > -    return 0;
> >  }
> >
> >  static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error
> **errp)
> > @@ -1734,12 +1733,10 @@ static void
> virtio_ccw_busdev_realize(DeviceState *dev, Error **errp)
> >      virtio_ccw_device_realize(_dev, errp);
> >  }
> >
> > -static int virtio_ccw_busdev_exit(DeviceState *dev)
> > +static void virtio_ccw_busdev_exit(DeviceState *dev)
> >  {
> >      VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
> >      VirtIOCCWDeviceClass *_info = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
> > -
> > -    return _info->exit(_dev);
> >  }
>
> These also are changing the implementation of a method but not its type.
>
> >
> >  static void virtio_ccw_busdev_unplug(HotplugHandler *hotplug_dev,
> > diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> > index 9ddd5ad..9962786 100644
> > --- a/hw/usb/ccid-card-emulated.c
> > +++ b/hw/usb/ccid-card-emulated.c
> > @@ -547,7 +547,7 @@ static int emulated_initfn(CCIDCardState *base)
> >      return 0;
> >  }
> >
> > -static int emulated_exitfn(CCIDCardState *base)
> > +static void emulated_exitfn(CCIDCardState *base)
> >  {
> >      EmulatedState *card = EMULATED_CCID_CARD(base);
> >      VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL);
> > @@ -564,7 +564,6 @@ static int emulated_exitfn(CCIDCardState *base)
> >      qemu_mutex_destroy(&card->handle_apdu_mutex);
> >      qemu_mutex_destroy(&card->vreader_mutex);
> >      qemu_mutex_destroy(&card->event_list_mutex);
> > -    return 0;
> >  }
> >
>
> as is this.
>
> >  static Property emulated_card_properties[] = {
> > diff --git a/hw/usb/dev-smartcard-reader.c
> b/hw/usb/dev-smartcard-reader.c
> > index 96a1a13..6cc18b1 100644
> > --- a/hw/usb/dev-smartcard-reader.c
> > +++ b/hw/usb/dev-smartcard-reader.c
> > @@ -507,14 +507,13 @@ static void
> ccid_card_apdu_from_guest(CCIDCardState *card,
> >      }
> >  }
> >
> > -static int ccid_card_exitfn(CCIDCardState *card)
> > +static void ccid_card_exitfn(CCIDCardState *card)
> >  {
> >      CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
> >
> >      if (cc->exitfn) {
> >          return cc->exitfn(card);
> >      }
> > -    return 0;
> >  }
> >
>
> This change has made the function's return type 'void' but
> there is still a line in it which is returning a value.
>
> Also, since the CCIDCardClass exitfn method returns a
> value, this function should too (its purpose is just to
> be a wrapper around the method call).
>
> >  static int ccid_card_initfn(CCIDCardState *card)
> > @@ -1276,9 +1275,8 @@ void ccid_card_card_inserted(CCIDCardState *card)
> >      ccid_on_slot_change(s, true);
> >  }
> >
> > -static int ccid_card_exit(DeviceState *qdev)
> > +static void ccid_card_exit(DeviceState *qdev)
> >  {
> > -    int ret = 0;
> >      CCIDCardState *card = CCID_CARD(qdev);
> >      USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> >      USBCCIDState *s = USB_CCID_DEV(dev);
> > @@ -1286,9 +1284,7 @@ static int ccid_card_exit(DeviceState *qdev)
> >      if (ccid_card_inserted(s)) {
> >          ccid_card_card_removed(card);
> >      }
> > -    ret = ccid_card_exitfn(card);
> >      s->card = NULL;
> > -    return ret;
>
> This change has lost the call to ccid_card_exitfn().
>
> >  }
> >
> >  static int ccid_card_init(DeviceState *qdev)
> > --
> > 1.9.1
> >
> >
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 10816 bytes --]

Re: [Qemu-devel] [PATCH] Change return type of functions that are named *_exit or *_exitfn in hw/ from int to void.

[Paolo Bonzini]

On 27/03/2016 10:00, Nutan Shinde wrote:
> Hi Peter,
> 
> I don't know why this change is required. I picked up this task from
> BitSizedTask. Can you please, tell me the purpose of this task?

It's just because the Error** is not used by anyone.

Of course, not understanding a task doesn't remove the need to
understand your patch.

Paolo

Re: [Qemu-devel] [PATCH] Change return type of functions that are named *_exit or *_exitfn in hw/ from int to void.

[Nutan Shinde]

[-- Attachment #1: Type: text/plain, Size: 8445 bytes --]

Hi,

> It's usually not a good idea to have one patch which touches
> multiple completely different devices or subsystems.

Shall I send multiple patches with subject [PATCH v2 1/3], [PATCH v2
2/3], [PATCH
v2 3/3] ?

Regards,
Nutan.

On Thu, Mar 24, 2016 at 11:49 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 24 March 2016 at 17:18, Nutan Shinde <nutanshinde1992@gmail.com> wrote:
>
> Hi; thanks for this patch; I have some comments below.
>
> > Functions that are named  *_exit or *_exitfn in hw/, all return 0.
> Changed return type of these methods from int to void.
> > Also, removed check where values were returned from these methods.
>
> In general, commit messages should describe the "why" of a
> change, not just the "how". They should also be line wrapped
> at about 70 characters.
>
>
> > Signed-off-by: Nutan Shinde <nutanshinde1992@gmail.com>
> > ---
> >  hw/audio/hda-codec.c          | 3 +--
> >  hw/audio/intel-hda.c          | 3 +--
> >  hw/char/sclpconsole-lm.c      | 3 +--
> >  hw/char/sclpconsole.c         | 3 +--
> >  hw/s390x/virtio-ccw.c         | 7 ++-----
> >  hw/usb/ccid-card-emulated.c   | 3 +--
> >  hw/usb/dev-smartcard-reader.c | 8 ++------
> >  7 files changed, 9 insertions(+), 21 deletions(-)
>
> It's usually not a good idea to have one patch which touches
> multiple completely different devices or subsystems.
>
> >
> > diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
> > index 52d4640..5402cd1 100644
> > --- a/hw/audio/hda-codec.c
> > +++ b/hw/audio/hda-codec.c
> > @@ -520,7 +520,7 @@ static int hda_audio_init(HDACodecDevice *hda, const
> struct desc_codec *desc)
> >      return 0;
> >  }
> >
> > -static int hda_audio_exit(HDACodecDevice *hda)
> > +static void hda_audio_exit(HDACodecDevice *hda)
> >  {
> >      HDAAudioState *a = HDA_AUDIO(hda);
> >      HDAAudioStream *st;
> > @@ -539,7 +539,6 @@ static int hda_audio_exit(HDACodecDevice *hda)
> >          }
> >      }
> >      AUD_remove_card(&a->card);
> > -    return 0;
> >  }
> >
>
> This function is used to initialize the 'exit' function pointer
> in the HDACodecDeviceClass structure, but you haven't changed the
> signature of that function pointer to match the change in the
> function type here. (I'm surprised this compiles.)
>
> >  static int hda_audio_post_load(void *opaque, int version)
> > diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> > index d372d4a..404cfcf 100644
> > --- a/hw/audio/intel-hda.c
> > +++ b/hw/audio/intel-hda.c
> > @@ -66,7 +66,7 @@ static int hda_codec_dev_init(DeviceState *qdev)
> >      return cdc->init(dev);
> >  }
> >
> > -static int hda_codec_dev_exit(DeviceState *qdev)
> > +static void hda_codec_dev_exit(DeviceState *qdev)
> >  {
> >      HDACodecDevice *dev = DO_UPCAST(HDACodecDevice, qdev, qdev);
> >      HDACodecDeviceClass *cdc = HDA_CODEC_DEVICE_GET_CLASS(dev);
> > @@ -74,7 +74,6 @@ static int hda_codec_dev_exit(DeviceState *qdev)
> >      if (cdc->exit) {
> >          cdc->exit(dev);
> >      }
> > -    return 0;
> >  }
> >
> >  HDACodecDevice *hda_codec_find(HDACodecBus *bus, uint32_t cad)
>
> This function is implementing the DeviceClass exit method, so you
> cannot just change its return type. Changing the method type would
> be OK I think (though probably we ought to convert to realize and
> unrealize in the longer term, so I'm not sure how much it's worth
> changing the type of the exit method.)
>
> > diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> > index 7d4ff81..d2fe30a 100644
> > --- a/hw/char/sclpconsole-lm.c
> > +++ b/hw/char/sclpconsole-lm.c
> > @@ -328,9 +328,8 @@ static int console_init(SCLPEvent *event)
> >      return 0;
> >  }
> >
> > -static int console_exit(SCLPEvent *event)
> > +static void console_exit(SCLPEvent *event)
> >  {
> > -    return 0;
> >  }
> >
> >  static void console_reset(DeviceState *dev)
> > diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
> > index 45997ff..da7b503 100644
> > --- a/hw/char/sclpconsole.c
> > +++ b/hw/char/sclpconsole.c
> > @@ -242,9 +242,8 @@ static void console_reset(DeviceState *dev)
> >     scon->notify = false;
> >  }
> >
> > -static int console_exit(SCLPEvent *event)
> > +static void console_exit(SCLPEvent *event)
> >  {
> > -    return 0;
> >  }
> >
> >  static Property console_properties[] = {
>
> These two functions are implementing the SCLPEventClass
> exit method, so you can't just change these without changing
> the method's type as well. (I don't know if that change is
> a good idea, the s390 maintainers would have to say.)
>
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index cb887ba..74e989b 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -877,7 +877,7 @@ out_err:
> >      g_free(sch);
> >  }
> >
> > -static int virtio_ccw_exit(VirtioCcwDevice *dev)
> > +static void virtio_ccw_exit(VirtioCcwDevice *dev)
> >  {
> >      SubchDev *sch = dev->sch;
> >
> > @@ -889,7 +889,6 @@ static int virtio_ccw_exit(VirtioCcwDevice *dev)
> >          release_indicator(&dev->routes.adapter, dev->indicators);
> >          dev->indicators = NULL;
> >      }
> > -    return 0;
> >  }
> >
> >  static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error
> **errp)
> > @@ -1734,12 +1733,10 @@ static void
> virtio_ccw_busdev_realize(DeviceState *dev, Error **errp)
> >      virtio_ccw_device_realize(_dev, errp);
> >  }
> >
> > -static int virtio_ccw_busdev_exit(DeviceState *dev)
> > +static void virtio_ccw_busdev_exit(DeviceState *dev)
> >  {
> >      VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
> >      VirtIOCCWDeviceClass *_info = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
> > -
> > -    return _info->exit(_dev);
> >  }
>
> These also are changing the implementation of a method but not its type.
>
> >
> >  static void virtio_ccw_busdev_unplug(HotplugHandler *hotplug_dev,
> > diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> > index 9ddd5ad..9962786 100644
> > --- a/hw/usb/ccid-card-emulated.c
> > +++ b/hw/usb/ccid-card-emulated.c
> > @@ -547,7 +547,7 @@ static int emulated_initfn(CCIDCardState *base)
> >      return 0;
> >  }
> >
> > -static int emulated_exitfn(CCIDCardState *base)
> > +static void emulated_exitfn(CCIDCardState *base)
> >  {
> >      EmulatedState *card = EMULATED_CCID_CARD(base);
> >      VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL);
> > @@ -564,7 +564,6 @@ static int emulated_exitfn(CCIDCardState *base)
> >      qemu_mutex_destroy(&card->handle_apdu_mutex);
> >      qemu_mutex_destroy(&card->vreader_mutex);
> >      qemu_mutex_destroy(&card->event_list_mutex);
> > -    return 0;
> >  }
> >
>
> as is this.
>
> >  static Property emulated_card_properties[] = {
> > diff --git a/hw/usb/dev-smartcard-reader.c
> b/hw/usb/dev-smartcard-reader.c
> > index 96a1a13..6cc18b1 100644
> > --- a/hw/usb/dev-smartcard-reader.c
> > +++ b/hw/usb/dev-smartcard-reader.c
> > @@ -507,14 +507,13 @@ static void
> ccid_card_apdu_from_guest(CCIDCardState *card,
> >      }
> >  }
> >
> > -static int ccid_card_exitfn(CCIDCardState *card)
> > +static void ccid_card_exitfn(CCIDCardState *card)
> >  {
> >      CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
> >
> >      if (cc->exitfn) {
> >          return cc->exitfn(card);
> >      }
> > -    return 0;
> >  }
> >
>
> This change has made the function's return type 'void' but
> there is still a line in it which is returning a value.
>
> Also, since the CCIDCardClass exitfn method returns a
> value, this function should too (its purpose is just to
> be a wrapper around the method call).
>
> >  static int ccid_card_initfn(CCIDCardState *card)
> > @@ -1276,9 +1275,8 @@ void ccid_card_card_inserted(CCIDCardState *card)
> >      ccid_on_slot_change(s, true);
> >  }
> >
> > -static int ccid_card_exit(DeviceState *qdev)
> > +static void ccid_card_exit(DeviceState *qdev)
> >  {
> > -    int ret = 0;
> >      CCIDCardState *card = CCID_CARD(qdev);
> >      USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> >      USBCCIDState *s = USB_CCID_DEV(dev);
> > @@ -1286,9 +1284,7 @@ static int ccid_card_exit(DeviceState *qdev)
> >      if (ccid_card_inserted(s)) {
> >          ccid_card_card_removed(card);
> >      }
> > -    ret = ccid_card_exitfn(card);
> >      s->card = NULL;
> > -    return ret;
>
> This change has lost the call to ccid_card_exitfn().
>
> >  }
> >
> >  static int ccid_card_init(DeviceState *qdev)
> > --
> > 1.9.1
> >
> >
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 11164 bytes --]

[Qemu-devel] [PATCH] Change return type of functions that are named *_exit or *_exitfn in hw/ from int to void

[Nutan Shinde]

This changes is required because the return type of exit(error) function is not used.
This patch only has changes for files in hw/audio package.

Signed-off-by: Nutan Shinde <nutanshinde1992@gmail.com>
---
 hw/audio/hda-codec.c   | 3 +--
 hw/audio/intel-hda.c   | 3 +--
 hw/audio/intel-hda.h   | 2 +-
 include/hw/qdev-core.h | 2 +-
 4 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index 52d4640..5402cd1 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -520,7 +520,7 @@ static int hda_audio_init(HDACodecDevice *hda, const struct desc_codec *desc)
     return 0;
 }
 
-static int hda_audio_exit(HDACodecDevice *hda)
+static void hda_audio_exit(HDACodecDevice *hda)
 {
     HDAAudioState *a = HDA_AUDIO(hda);
     HDAAudioStream *st;
@@ -539,7 +539,6 @@ static int hda_audio_exit(HDACodecDevice *hda)
         }
     }
     AUD_remove_card(&a->card);
-    return 0;
 }
 
 static int hda_audio_post_load(void *opaque, int version)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index d372d4a..404cfcf 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -66,7 +66,7 @@ static int hda_codec_dev_init(DeviceState *qdev)
     return cdc->init(dev);
 }
 
-static int hda_codec_dev_exit(DeviceState *qdev)
+static void hda_codec_dev_exit(DeviceState *qdev)
 {
     HDACodecDevice *dev = DO_UPCAST(HDACodecDevice, qdev, qdev);
     HDACodecDeviceClass *cdc = HDA_CODEC_DEVICE_GET_CLASS(dev);
@@ -74,7 +74,6 @@ static int hda_codec_dev_exit(DeviceState *qdev)
     if (cdc->exit) {
         cdc->exit(dev);
     }
-    return 0;
 }
 
 HDACodecDevice *hda_codec_find(HDACodecBus *bus, uint32_t cad)
diff --git a/hw/audio/intel-hda.h b/hw/audio/intel-hda.h
index d784bcf..53b78da 100644
--- a/hw/audio/intel-hda.h
+++ b/hw/audio/intel-hda.h
@@ -38,7 +38,7 @@ typedef struct HDACodecDeviceClass
     DeviceClass parent_class;
 
     int (*init)(HDACodecDevice *dev);
-    int (*exit)(HDACodecDevice *dev);
+    void (*exit)(HDACodecDevice *dev);
     void (*command)(HDACodecDevice *dev, uint32_t nid, uint32_t data);
     void (*stream)(HDACodecDevice *dev, uint32_t stnr, bool running, bool output);
 } HDACodecDeviceClass;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1ce02b2..0b635bd 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -30,7 +30,7 @@ typedef enum DeviceCategory {
 } DeviceCategory;
 
 typedef int (*qdev_initfn)(DeviceState *dev);
-typedef int (*qdev_event)(DeviceState *dev);
+typedef void (*qdev_event)(DeviceState *dev);
 typedef void (*qdev_resetfn)(DeviceState *dev);
 typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
 typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp);
-- 
1.9.1

Re: [Qemu-devel] [PATCH] Change return type of functions that are named *_exit or *_exitfn in hw/ from int to void

[Nutan Shinde]

[-- Attachment #1: Type: text/plain, Size: 3130 bytes --]

Hi,

Please ignore this patch, I have sent a new patch with version v2, in
subject line, which has updated changes.

Regards,
Nutan.

On Tue, Apr 12, 2016 at 11:24 PM, Nutan Shinde <nutanshinde1992@gmail.com>
wrote:

> This changes is required because the return type of exit(error) function
> is not used.
> This patch only has changes for files in hw/audio package.
>
> Signed-off-by: Nutan Shinde <nutanshinde1992@gmail.com>
> ---
>  hw/audio/hda-codec.c   | 3 +--
>  hw/audio/intel-hda.c   | 3 +--
>  hw/audio/intel-hda.h   | 2 +-
>  include/hw/qdev-core.h | 2 +-
>  4 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
> index 52d4640..5402cd1 100644
> --- a/hw/audio/hda-codec.c
> +++ b/hw/audio/hda-codec.c
> @@ -520,7 +520,7 @@ static int hda_audio_init(HDACodecDevice *hda, const
> struct desc_codec *desc)
>      return 0;
>  }
>
> -static int hda_audio_exit(HDACodecDevice *hda)
> +static void hda_audio_exit(HDACodecDevice *hda)
>  {
>      HDAAudioState *a = HDA_AUDIO(hda);
>      HDAAudioStream *st;
> @@ -539,7 +539,6 @@ static int hda_audio_exit(HDACodecDevice *hda)
>          }
>      }
>      AUD_remove_card(&a->card);
> -    return 0;
>  }
>
>  static int hda_audio_post_load(void *opaque, int version)
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index d372d4a..404cfcf 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -66,7 +66,7 @@ static int hda_codec_dev_init(DeviceState *qdev)
>      return cdc->init(dev);
>  }
>
> -static int hda_codec_dev_exit(DeviceState *qdev)
> +static void hda_codec_dev_exit(DeviceState *qdev)
>  {
>      HDACodecDevice *dev = DO_UPCAST(HDACodecDevice, qdev, qdev);
>      HDACodecDeviceClass *cdc = HDA_CODEC_DEVICE_GET_CLASS(dev);
> @@ -74,7 +74,6 @@ static int hda_codec_dev_exit(DeviceState *qdev)
>      if (cdc->exit) {
>          cdc->exit(dev);
>      }
> -    return 0;
>  }
>
>  HDACodecDevice *hda_codec_find(HDACodecBus *bus, uint32_t cad)
> diff --git a/hw/audio/intel-hda.h b/hw/audio/intel-hda.h
> index d784bcf..53b78da 100644
> --- a/hw/audio/intel-hda.h
> +++ b/hw/audio/intel-hda.h
> @@ -38,7 +38,7 @@ typedef struct HDACodecDeviceClass
>      DeviceClass parent_class;
>
>      int (*init)(HDACodecDevice *dev);
> -    int (*exit)(HDACodecDevice *dev);
> +    void (*exit)(HDACodecDevice *dev);
>      void (*command)(HDACodecDevice *dev, uint32_t nid, uint32_t data);
>      void (*stream)(HDACodecDevice *dev, uint32_t stnr, bool running, bool
> output);
>  } HDACodecDeviceClass;
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 1ce02b2..0b635bd 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -30,7 +30,7 @@ typedef enum DeviceCategory {
>  } DeviceCategory;
>
>  typedef int (*qdev_initfn)(DeviceState *dev);
> -typedef int (*qdev_event)(DeviceState *dev);
> +typedef void (*qdev_event)(DeviceState *dev);
>  typedef void (*qdev_resetfn)(DeviceState *dev);
>  typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
>  typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp);
> --
> 1.9.1
>
>

[-- Attachment #2: Type: text/html, Size: 3946 bytes --]