| Message ID | cover.1616521341.git.lukasstraub2@web.de |
|---|---|
| Headers | show |
| Series | yank: Always link full yank code | expand |
On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote: > Hello Everyone, > These patches remove yank's dependency on qiochannel and always link it in. > Please Review. It would be useful if the cover letter or commit messages explained to potential reviewers why this is being changed... The first patch feels like a regression to me, without seeing an explanation why it is desirable. Regards, Daniel
On Tue, 23 Mar 2021 19:09:15 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote: > > Hello Everyone, > > These patches remove yank's dependency on qiochannel and always link it in. > > Please Review. > > It would be useful if the cover letter or commit messages explained > to potential reviewers why this is being changed... The first patch > feels like a regression to me, without seeing an explanation why > it is desirable. Yes, sorry. There are two reasons for this patchset: -To exercise the full yank code (with checks for registering and unregistering of yank functions and instances) in existing tests and in the new yank test (https://lore.kernel.org/qemu-devel/cover.1616521487.git.lukasstraub2@web.de/) -To replace "[PATCH] yank: Avoid linking into executables that don't want it" (https://lore.kernel.org/qemu-devel/20210316135907.3646901-1-armbru@redhat.com/) Now we always link in yank, but without pulling in other dependencies. Regards, Lukas Straub > > Regards, > Daniel --
On Wed, Mar 24, 2021 at 12:22:42PM +0100, Lukas Straub wrote: > On Tue, 23 Mar 2021 19:09:15 +0000 > Daniel P. Berrangé <berrange@redhat.com> wrote: > > > On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote: > > > Hello Everyone, > > > These patches remove yank's dependency on qiochannel and always link it in. > > > Please Review. > > > > It would be useful if the cover letter or commit messages explained > > to potential reviewers why this is being changed... The first patch > > feels like a regression to me, without seeing an explanation why > > it is desirable. > > Yes, sorry. There are two reasons for this patchset: > -To exercise the full yank code (with checks for registering and unregistering > of yank functions and instances) in existing tests and in the new yank test > (https://lore.kernel.org/qemu-devel/cover.1616521487.git.lukasstraub2@web.de/) > -To replace "[PATCH] yank: Avoid linking into executables that don't want it" > (https://lore.kernel.org/qemu-devel/20210316135907.3646901-1-armbru@redhat.com/) > Now we always link in yank, but without pulling in other dependencies. Conceptually, the real world usage of yank is in combination with parts of QEMU doing I/O, and so they will always have the "io" subsystem available. Thus it feels wrong to be refactoring this to avoid an "io" dependancy. I think this probably suggests that the yank code shouldn't be in libqemuutil.la, but instead part of the "io" subsystem to make the association/dependency explicit Regards, Daniel
On Wed, 24 Mar 2021 11:36:13 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Wed, Mar 24, 2021 at 12:22:42PM +0100, Lukas Straub wrote: > > On Tue, 23 Mar 2021 19:09:15 +0000 > > Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote: > > > > Hello Everyone, > > > > These patches remove yank's dependency on qiochannel and always link it in. > > > > Please Review. > > > > > > It would be useful if the cover letter or commit messages explained > > > to potential reviewers why this is being changed... The first patch > > > feels like a regression to me, without seeing an explanation why > > > it is desirable. > > > > Yes, sorry. There are two reasons for this patchset: > > -To exercise the full yank code (with checks for registering and unregistering > > of yank functions and instances) in existing tests and in the new yank test > > (https://lore.kernel.org/qemu-devel/cover.1616521487.git.lukasstraub2@web.de/) > > -To replace "[PATCH] yank: Avoid linking into executables that don't want it" > > (https://lore.kernel.org/qemu-devel/20210316135907.3646901-1-armbru@redhat.com/) > > Now we always link in yank, but without pulling in other dependencies. > > Conceptually, the real world usage of yank is in combination with parts > of QEMU doing I/O, and so they will always have the "io" subsystem > available. Thus it feels wrong to be refactoring this to avoid an > "io" dependancy. I think this probably suggests that the yank code > shouldn't be in libqemuutil.la, but instead part of the "io" subsystem > to make the association/dependency explicit Yes, makes sense. On the other hand I think that the yank functions should be separate from the yank core anyway. Regards, Lukas Straub > > Regards, > Daniel --