| Message ID | 1458839897-4609-1-git-send-email-nutanshinde1992@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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 >
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)
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(-)