Re: [PATCH v7 10/13] migrate: Update protocol to v3 for multi-address support

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8151 bytes --]

On Sun, Apr 12, 2026 at 08:53:16PM -0400, Jon Maloy wrote:
> We update the migration protocol to version 3 to support distributing
> multiple addresses from the unified address array. The new protocol
> migrates all address entries in the array, along with their prefix
> lengths and flags, and leaves it to the receiver to filter which
> ones he wants to apply.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> 
> ---
> v4: - Broke out as separate commit
>     - Made number of transferable addresses variable
> 
> v6: - Separated internal and wire transfer format
> 
> v7: - Using uint32_t instead of uint8_t for fields in migration format
>     - Replaced term "wire format" with "migration format"
>     - Some other minor changes after feedback from Stefano.
> ---
>  migrate.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 179 insertions(+)
> 
> diff --git a/migrate.c b/migrate.c
> index 2dc4dd9..93f67ae 100644
> --- a/migrate.c
> +++ b/migrate.c
> @@ -44,6 +44,71 @@ struct migrate_seen_addrs_v2 {
>  	unsigned char mac[ETH_ALEN];
>  } __attribute__((packed));
>  
> +/**
> + * Migration format flags for address migration (v3)
> + * These are stable values - do not change existing assignments
> + */
> +#define MIGRATE_ADDR_USER	BIT(0)
> +#define MIGRATE_ADDR_HOST	BIT(1)
> +#define MIGRATE_ADDR_LINKLOCAL	BIT(2)
> +#define MIGRATE_ADDR_OBSERVED	BIT(3)
> +
> +/**
> + * struct migrate_addr_v3 - Migration format for a single address entry
> + * @addr:	IPv6 or IPv4-mapped address (16 bytes)
> + * @prefix_len:	Prefix length
> + * @flags:	MIGRATE_ADDR_* flags (migration format)
> + */
> +struct migrate_addr_v3 {
> +	struct in6_addr addr;
> +	uint32_t prefix_len;
> +	uint32_t flags;

Since this is 32-bit...

> +} __attribute__((__packed__));
> +
> +/**
> + * flags_to_migration() - Convert internal flags to stable migration format
> + * @flags:	Internal CONF_ADDR_* flags
> + *
> + * Return: Migration format MIGRATE_ADDR_* flags
> + */
> +static uint8_t flags_to_migration(uint8_t flags)

... this should probably also return 32-bit.

> +{
> +	uint8_t migration = 0;
> +
> +	if (flags & CONF_ADDR_USER)
> +		migration |= MIGRATE_ADDR_USER;
> +	if (flags & CONF_ADDR_HOST)
> +		migration |= MIGRATE_ADDR_HOST;
> +	if (flags & CONF_ADDR_LINKLOCAL)
> +		migration |= MIGRATE_ADDR_LINKLOCAL;
> +	if (flags & CONF_ADDR_OBSERVED)
> +		migration |= MIGRATE_ADDR_OBSERVED;
> +
> +	return migration;

That way you could also include the htonl() / ntohl() as part of the
conversion functions.

> +}
> +
> +/**
> + * flags_from_migration() - Convert migration format flags to internal format
> + * @migration:	Migration format MIGRATE_ADDR_* flags
> + *
> + * Return: Internal CONF_ADDR_* flags
> + */
> +static uint8_t flags_from_migration(uint8_t migration)

Same comments here, but in reverse.

> +{
> +	uint8_t flags = 0;
> +
> +	if (migration & MIGRATE_ADDR_USER)
> +		flags |= CONF_ADDR_USER;
> +	if (migration & MIGRATE_ADDR_HOST)
> +		flags |= CONF_ADDR_HOST;
> +	if (migration & MIGRATE_ADDR_LINKLOCAL)
> +		flags |= CONF_ADDR_LINKLOCAL;
> +	if (migration & MIGRATE_ADDR_OBSERVED)
> +		flags |= CONF_ADDR_OBSERVED;
> +
> +	return flags;
> +}
> +
>  /**
>   * seen_addrs_source_v2() - Copy and send guest observed addresses from source
>   * @c:		Execution context
> @@ -126,6 +191,99 @@ static int seen_addrs_target_v2(struct ctx *c,
>  	return 0;
>  }
>  
> +/**
> + * addrs_source_v3() - Send all addresses with flags from source
> + * @c:		Execution context
> + * @stage:	Migration stage, unused
> + * @fd:		File descriptor for state transfer
> + *
> + * Send all address entries using a stable migration format. Each field is
> + * serialised explicitly to avoid coupling the migration format to internal
> + * structure layout or flag bit assignments.
> + *
> + * Return: 0 on success, positive error code on failure
> + */
> +/* cppcheck-suppress [constParameterCallback, unmatchedSuppression] */
> +static int addrs_source_v3(struct ctx *c,
> +			   const struct migrate_stage *stage, int fd)
> +{
> +	uint8_t addr_count = c->addr_count;
> +	const struct guest_addr *a;
> +
> +	(void)stage;
> +
> +	/* Send count first */
> +	if (write_all_buf(fd, &addr_count, sizeof(addr_count)))
> +		return errno;

I'd be inclined to use 32-bits here, rather than 8.  Yes, 255 is
probably more than enough addresses, but this gains future-proofness
and consistency with other counts we send at negligible cost.

Either way you can use write_u8() or write_u32() from serialise.c
here.

> +
> +	/* Send each address in stable migration format */
> +	for_each_addr(a, c->addrs, c->addr_count, 0) {
> +		struct migrate_addr_v3 migration = {
> +			.addr = a->addr.a6,
> +			.prefix_len = htonl(a->prefix_len),
> +			.flags = htonl(flags_to_migration(a->flags)),
> +		};
> +
> +		if (write_all_buf(fd, &migration, sizeof(migration)))
> +			return errno;
> +	}
> +
> +	/* Send MAC address */
> +	if (write_all_buf(fd, c->guest_mac, ETH_ALEN))
> +		return errno;
> +
> +	return 0;
> +}
> +
> +/**
> + * addrs_target_v3() - Receive addresses on target
> + * @c:		Execution context
> + * @stage:	Migration stage, unused
> + * @fd:		File descriptor for state transfer
> + *
> + * Receive address entries from the stable migration format and merge only
> + * observed addresses into local array. Source sends all addresses for
> + * forward compatibility, but target only applies those marked as observed.
> + *
> + * Return: 0 on success, positive error code on failure
> + */
> +static int addrs_target_v3(struct ctx *c,
> +			   const struct migrate_stage *stage, int fd)
> +{
> +	uint8_t addr_count, i;
> +
> +	(void)stage;
> +
> +	if (read_all_buf(fd, &addr_count, sizeof(addr_count)))
> +		return errno;

read_u8() / read_u32() from serialise.c.

> +
> +	if (addr_count > MAX_GUEST_ADDRS)

Print a warning?

> +		addr_count = MAX_GUEST_ADDRS;
> +
> +	/* Read each address from stable migration format */
> +	for (i = 0; i < addr_count; i++) {
> +		struct migrate_addr_v3 migration;
> +		struct guest_addr addr;
> +
> +		if (read_all_buf(fd, &migration, sizeof(migration)))
> +			return errno;
> +
> +		addr.addr.a6 = migration.addr;
> +		addr.prefix_len = ntohl(migration.prefix_len);
> +		addr.flags = flags_from_migration(ntohl(migration.flags));
> +
> +		if (addr.flags & CONF_ADDR_OBSERVED) {
> +			fwd_set_addr(c, &addr.addr, addr.flags,
> +				     addr.prefix_len);
> +		}

I'm assuming that sending all the addresses, but only importing the
OBSERVED ones is to allow us to change the policy about which
addresses are migrated in future.  That seems wise.

> +	}
> +
> +	if (read_all_buf(fd, c->guest_mac, ETH_ALEN))
> +		return errno;
> +
> +	return 0;
> +}
> +
>  /* Stages for version 2 */
>  static const struct migrate_stage stages_v2[] = {
>  	{
> @@ -146,8 +304,29 @@ static const struct migrate_stage stages_v2[] = {
>  	{ 0 },
>  };
>  
> +/* Stages for version 3 (all addresses, with flags) */
> +static const struct migrate_stage stages_v3[] = {
> +	{
> +		.name = "addresses",
> +		.source = addrs_source_v3,
> +		.target = addrs_target_v3,
> +	},
> +	{
> +		.name = "prepare flows",
> +		.source = flow_migrate_source_pre,
> +		.target = NULL,
> +	},
> +	{
> +		.name = "transfer flows",
> +		.source = flow_migrate_source,
> +		.target = flow_migrate_target,
> +	},
> +	{ 0 },
> +};
> +
>  /* Supported encoding versions, from latest (most preferred) to oldest */
>  static const struct migrate_version versions[] = {
> +	{ 3,	stages_v3, },
>  	{ 2,	stages_v2, },
>  	/* v1 was released, but not widely used.  It had bad endianness for the
>  	 * MSS and omitted timestamps, which meant it usually wouldn't work.
> -- 
> 2.52.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]