Re: [Qemu-devel] [PATCH 3/9] migration: Export qemu-file-channel.c functions in its own file

[Peter Xu <peterx@redhat.com>]

On Thu, May 18, 2017 at 03:26:23PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Wed, May 17, 2017 at 05:47:50PM +0200, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  include/migration/migration.h |  1 +
> >>  include/migration/qemu-file.h |  4 ----
> >>  migration/channel.c           |  1 +
> >>  migration/colo.c              |  1 +
> >>  migration/migration.c         |  1 +
> >>  migration/qemu-file-channel.c |  1 +
> >>  migration/qemu-file-channel.h | 21 +++++++++++++++++++++
> >>  migration/rdma.c              |  1 +
> >>  migration/savevm.c            |  1 +
> >>  tests/test-vmstate.c          |  1 +
> >>  10 files changed, 29 insertions(+), 4 deletions(-)
> >>  create mode 100644 migration/qemu-file-channel.h
> >> 
> >> diff --git a/include/migration/migration.h b/include/migration/migration.h
> >> index e831259..8280df1 100644
> >> --- a/include/migration/migration.h
> >> +++ b/include/migration/migration.h
> >> @@ -19,6 +19,7 @@
> >>  #include "qemu/thread.h"
> >>  #include "qemu/notify.h"
> >>  #include "migration/vmstate.h"
> >> +#include "io/channel.h"
> >
> > Could I ask why we add this line here? I thought one of the main goals
> > of this series is removing things from migration.h...
> 
> 
> 
> I remove from include/migration/qemu-file.h
> 
> -#include "io/channel.h"
> 
> 
> Because all the QIOChannel functions in qemu-file.h are moved to
> qemu-file-channel.h.
> 
> Great!
> 
> But migration/vmstate.h includes qemu-file.h
> 
> And migration.h includes vmstate.h
> 
> And migration.h has this functions:
> 
> void qemu_start_incoming_migration(const char *uri, Error **errp);
>                                             QIOChannel *ioc,
>                                             Error **errp);

(In my repo, it does not need QIOChannel, which looks like:
 void qemu_start_incoming_migration(const char *uri, Error **errp)
 but it does not really matter much...)

> 
> void migration_tls_channel_connect(MigrationState *s,
>                                    QIOChannel *ioc,
>                                    const char *hostname,
>                                    Error **errp);
> 
> And nothing else declares the QIOChannel.
> 
> So, the easy solution so far is to include this by now to maintain
> compilation.

I see. It's okay to me.

(Another solution would be moving these functions outside of
 migration/migration.h as well since they are used by migration
 internally as well? Anyway we already have migration/tls.c to keep
 migration_tls_* functions)

I'll reply to the latest version of this patch for the r-b. Thanks.

-- 
Peter Xu