Re: [PATCH v3 5/5] tools/xenstore: add migration stream extensions for new features

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 08/08/2022 12:31, Juergen Gross wrote:
> On 08.08.22 13:00, Julien Grall wrote:
> > > This would break the use of xenstore-stubdom for such a setup.
> > I am not sure why it would break the use of xenstore-stubdom. An 
> > application will already need to cope with the case Xenstored doesn't 
> > support a feature.
> Someone relying to be able to switch off a feature on a socket connection
> might get into trouble trying to do the same when running with 
> xenstore-stubdom.
This is not very different from an application that was built against an 
old Xenstored and would not be capable to talk properly with the new 
Xenstored if the feature is enabled. I understand that...
> Switching a feature off will either not work, or switch the feature off 
> for all
> dom0 connections (which is a single one, of course).
... when using xenstore-stubdom xenstored it means that the feature will 
have to be disable for all dom0 connections.
However, it seems unlikely to me that someone will switch to a 
xenstore-stubdom on a whim because there are also scalability concerns 
(one ring to rule all connections). So I think it would be fair to say 
that your application may need to be tweak if you are not using the same 
feature as the system.
> > At which point, it would be easy to say "I don't want this feature" 
> > when using a socket.
> I don't see the value of that. If you don't want a feature, just don't 
> use it.
This is not that simple. Your assumption is the feature will not change 
the behavior exposed to the application.
I don't think we have such feature today but I don't see what prevents 
us to do that.
> > > > I can't make my mind yet if we can avoid bumping the version for 
> > > > this field. What would happen if we need to rollback?
> > > I think an active usage of the new features and a rollback are mutually
> > > exclusive. See above.
> > > > > +|                |                                              |
> > > > > +| `quota-names`  | 0 delimited strings of the quota names in    |
> > > > > +|                | the same sequence as the `quota-val` values. |
> > > > > +|                | Only for `conn-type` 0 (shared ring).        |
> > > > > +|                | Only valid for version 2 and later.          |
> > > > As for the "global" quotas, I would move the quotas in a separate 
> > > > record. In this case, this would also be useful to avoid having may 
> > > > dynamic length field within the same record.
> > > I like having the data together more.
> > Which is fine so long the code doesn't become too horrible to 
> > read/maintain. I think having dynamic length array in the middle of 
> > the record makes it trickier.
> > This will only become worse as we introduce new fields in newer 
> > revision. So at which point would you say the record has grown too much?
> > To me, this is already the point and we have plenty of record ID to 
> > handle that.
> Fair enough.
>
> Other questions arising from that:
>
> - Should we have different record types for global and per-domain quota?
Given the question below, I guess you mean per-domain quotas that are 
not the default ones. If yes, then they should be split.
> - Should we split global quota into two record types (per-domain settable
>    and global acting ones)?
I don't have a strong opinion on this.

> - Combination of above (one record type for per-domain quota, usable for
>    global default with "invalid domid", and one record type for the global
>    acting ones like max. path-length)?
[...]

> > > I thought about that, but liked it better to be able to keep a common 
> > > struct
> > > layout for the record with the V2 fields being at the end.
> > >
> > > Main reason is the ability to avoid duplication of code for being 
> > > able to
> > > handle both versions.
> > The cons is you can't easily describe the record in "struct ...". As I 
> > wrote above, I think have dynamic length array in the middle of a 
> > record is wrong.
> You've got a point here.
>
> > I have looked at the code, I don't think there will be enough code 
> > duplication to warrant adding fixed field at the end of the record.
> Okay, lets go with a new record type then.
>
> Should that always be used, or only if the depth information has been
> specified (IOW: is the old watch record format invalid in V2)?
I don't have a strong opinion.

Cheers,

--
Julien Grall