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

[Juan Quintela]

Peter Xu <address@hidden> wrote:
> On Thu, May 18, 2017 at 03:26:23PM +0200, Juan Quintela wrote:
>> Peter Xu <address@hidden> wrote:
>> > On Wed, May 17, 2017 at 05:47:50PM +0200, Juan Quintela wrote:
>> >> Signed-off-by: Juan Quintela <address@hidden>
>> >> ---
>> >>  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.

They are moved there on the new cleanup series that I sent on Thrusday.
Internally the series were about 70 patches, so I tried to group the
patches in series by the way that they do similar things, so some things
like this one have not been handled "perfectly".  Grouping by this kind
of thing made other things less clean.  Compromise, as usual.

Thanks, Juan.