| Message ID | 1493211188-24086-1-git-send-email-tgnyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/26/2017 07:53 AM, Zihan Yang wrote: > Currently, the console_exit function in sclpconsole-lm.c and sclpconsole.c > does nothing, so remove them and convert the callback in SCLPEventClass to > void. Since there is a NULL check on the DeviceClass exit callback as > suggested by Frederic Konrad, it should be safe to remove them. When sending a patch series, please be sure to include a 0/5 cover letter, to make it easier on automated tooling that processes the list. You can use 'git config format.coverletter auto' to make it easier to remember this for patches created by 'git format-patch'/'git send-email'. More submission hints at: http://wiki.qemu.org/Contribute/SubmitAPatch
On Wed, 26 Apr 2017 09:42:02 -0500 Eric Blake <eblake@redhat.com> wrote: > On 04/26/2017 07:53 AM, Zihan Yang wrote: > > Currently, the console_exit function in sclpconsole-lm.c and sclpconsole.c > > does nothing, so remove them and convert the callback in SCLPEventClass to > > void. Since there is a NULL check on the DeviceClass exit callback as > > suggested by Frederic Konrad, it should be safe to remove them. > > When sending a patch series, please be sure to include a 0/5 cover > letter, to make it easier on automated tooling that processes the list. > You can use 'git config format.coverletter auto' to make it easier to > remember this for patches created by 'git format-patch'/'git send-email'. > > More submission hints at: http://wiki.qemu.org/Contribute/SubmitAPatch > Nod. Of the hints in the wiki page above, I'd especially like to point out versioning and change logs.
OK, sorry for the confusion, I will give a new patch series. I'm not very familiar with the conventions so I wonder if someone could help clarify some principles for me. I'd like to replace all the init/exit callback of DeviceClass to realize/unrealize, and convert return type of exit callback of some higher-level classes, like HDACodecDeviceClass, to void. My first question Is it a good idea to split these patches into two series? For example, one series to convert exit callback of these higher-level classes to void, and then the other to replace all the init/exit callback of DeviceClass to realize/unrealize. The second question is that should I always give separate patch for different directories? One example is that in both hw/ide and hw/block, I need to replace the init callback with realize in some high-level classes, should I give two patches or just give one patch for the work? Thanks
On Thu, 27 Apr 2017 20:23:51 +0800 Zihan Yang <tgnyang@gmail.com> wrote: > OK, sorry for the confusion, I will give a new patch series. I'm not very > familiar with > the conventions so I wonder if someone could help clarify some principles > for me. > I'd like to replace all the init/exit callback of DeviceClass to > realize/unrealize, and > convert return type of exit callback of some higher-level classes, like > HDACodecDeviceClass, to void. > > My first question Is it a good idea to split these patches into two series? > For example, > one series to convert exit callback of these higher-level classes to void, > and then the > other to replace all the init/exit callback of DeviceClass to > realize/unrealize. This is probably a good idea: converting exit callbacks to void is relatively straightforward, while converting to realize/unrealize might be better done while revisiting some of the modelling of the relevant subsystems. (For example, all classes for virtio-ccw use the same exit callback - it might be a good idea to do the same work in a common unrealize function when you touch it anyway.) > > The second question is that should I always give separate patch for > different directories? > One example is that in both hw/ide and hw/block, I need to replace the > init callback with > realize in some high-level classes, should I give two patches or just give > one patch for > the work? A better way to split is along maintainership responsibilites (which may or may not align with directories). Again, sticking with the code I maintain, I would merge changes to hw/char/sclp* and hw/s390x/, but not to other code in hw/char/. Try to stick to logical units (continuing with that example, different patches for sclp and virtio-ccw would make sense). The most important thing to remember when splitting changes is that you need to preserve bisectability, i.e. every step in your patch series needs to compile.
diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c index 07d6ebd..86ddda6 100644 --- a/hw/char/sclpconsole-lm.c +++ b/hw/char/sclpconsole-lm.c @@ -318,11 +318,6 @@ static int console_init(SCLPEvent *event) return 0; } -static int console_exit(SCLPEvent *event) -{ - return 0; -} - static void console_reset(DeviceState *dev) { SCLPEvent *event = SCLP_EVENT(dev); @@ -349,7 +344,6 @@ static void console_class_init(ObjectClass *klass, void *data) dc->reset = console_reset; dc->vmsd = &vmstate_sclplmconsole; ec->init = console_init; - ec->exit = console_exit; ec->get_send_mask = send_mask; ec->get_receive_mask = receive_mask; ec->can_handle_event = can_handle_event; diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c index b78f240..e916cac 100644 --- a/hw/char/sclpconsole.c +++ b/hw/char/sclpconsole.c @@ -246,11 +246,6 @@ static void console_reset(DeviceState *dev) scon->notify = false; } -static int console_exit(SCLPEvent *event) -{ - return 0; -} - static Property console_properties[] = { DEFINE_PROP_CHR("chardev", SCLPConsole, chr), DEFINE_PROP_END_OF_LIST(), @@ -265,7 +260,6 @@ static void console_class_init(ObjectClass *klass, void *data) dc->reset = console_reset; dc->vmsd = &vmstate_sclpconsole; ec->init = console_init; - ec->exit = console_exit; ec->get_send_mask = send_mask; ec->get_receive_mask = receive_mask; ec->can_handle_event = can_handle_event; diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c index 34b2faf..f7c509c 100644 --- a/hw/s390x/event-facility.c +++ b/hw/s390x/event-facility.c @@ -413,11 +413,7 @@ static void event_unrealize(DeviceState *qdev, Error **errp) SCLPEvent *event = SCLP_EVENT(qdev); SCLPEventClass *child = SCLP_EVENT_GET_CLASS(event); if (child->exit) { - int rc = child->exit(event); - if (rc < 0) { - error_setg(errp, "SCLP event exit failed."); - return; - } + child->exit(event); } } diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h index def1bb0..1a32f3a 100644 --- a/include/hw/s390x/event-facility.h +++ b/include/hw/s390x/event-facility.h @@ -162,7 +162,7 @@ typedef struct SCLPEvent { typedef struct SCLPEventClass { DeviceClass parent_class; int (*init)(SCLPEvent *event); - int (*exit)(SCLPEvent *event); + void (*exit)(SCLPEvent *event); /* get SCLP's send mask */ unsigned int (*get_send_mask)(void);
Currently, the console_exit function in sclpconsole-lm.c and sclpconsole.c does nothing, so remove them and convert the callback in SCLPEventClass to void. Since there is a NULL check on the DeviceClass exit callback as suggested by Frederic Konrad, it should be safe to remove them. Signed-off-by: Zihan Yang <tgnyang@gmail.com> --- hw/char/sclpconsole-lm.c | 6 ------ hw/char/sclpconsole.c | 6 ------ hw/s390x/event-facility.c | 6 +----- include/hw/s390x/event-facility.h | 2 +- 4 files changed, 2 insertions(+), 18 deletions(-)