1. Patchew
  2. QEMU
  3. View series

[PATCH 0/2] yank: Always link full yank code

Lukas Straub posted 2 patches 3 years, 1 month ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/cover.1616521341.git.lukasstraub2@web.de
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Juan Quintela <quintela@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Lukas Straub <lukasstraub2@web.de>
MAINTAINERS                   |  2 +-
chardev/char-socket.c         | 21 ++++++++++++++-------
include/qemu/yank.h           | 10 ----------
migration/channel.c           |  6 ++++--
migration/meson.build         |  1 +
migration/multifd.c           |  3 ++-
migration/qemu-file-channel.c |  3 ++-
migration/yank_functions.c    | 20 ++++++++++++++++++++
migration/yank_functions.h    | 17 +++++++++++++++++
stubs/meson.build             |  1 -
stubs/yank.c                  | 29 -----------------------------
util/meson.build              |  2 +-
util/yank.c                   |  8 --------
13 files changed, 62 insertions(+), 61 deletions(-)
create mode 100644 migration/yank_functions.c
create mode 100644 migration/yank_functions.h
delete mode 100644 stubs/yank.c
[PATCH 0/2] yank: Always link full yank code
Posted by Lukas Straub 3 years, 1 month ago 
Hello Everyone,
These patches remove yank's dependency on qiochannel and always link it in.
Please Review.

Regards,
Lukas Straub

Lukas Straub (2):
  yank: Remove dependency on qiochannel
  yank: Always link full yank code

 MAINTAINERS                   |  2 +-
 chardev/char-socket.c         | 21 ++++++++++++++-------
 include/qemu/yank.h           | 10 ----------
 migration/channel.c           |  6 ++++--
 migration/meson.build         |  1 +
 migration/multifd.c           |  3 ++-
 migration/qemu-file-channel.c |  3 ++-
 migration/yank_functions.c    | 20 ++++++++++++++++++++
 migration/yank_functions.h    | 17 +++++++++++++++++
 stubs/meson.build             |  1 -
 stubs/yank.c                  | 29 -----------------------------
 util/meson.build              |  2 +-
 util/yank.c                   |  8 --------
 13 files changed, 62 insertions(+), 61 deletions(-)
 create mode 100644 migration/yank_functions.c
 create mode 100644 migration/yank_functions.h
 delete mode 100644 stubs/yank.c

--
2.30.2
Re: [PATCH 0/2] yank: Always link full yank code
Posted by Daniel P. Berrangé 3 years, 1 month ago 
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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH 0/2] yank: Always link full yank code
Posted by Lukas Straub 3 years, 1 month ago 
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



--
Re: [PATCH 0/2] yank: Always link full yank code
Posted by Daniel P. Berrangé 3 years, 1 month ago 
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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH 0/2] yank: Always link full yank code
Posted by Lukas Straub 3 years, 1 month ago 
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



--

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.