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

[Nutan Shinde]

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 <address@hidden> wrote:

On 24 March 2016 at 17:18, Nutan Shinde <address@hidden> 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 <address@hidden>
> ---
>  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