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.mayd...@linaro.org>
wrote:
> On 24 March 2016 at 17:18, Nutan Shinde <nutanshinde1...@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 <nutanshinde1...@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
>
