Re: [PATCH v4 4/4] tools/xenstore: add migration stream extensions for new features

[Juergen Gross <jgross@xxxxxxxx>]

On 07.09.22 10:44, Julien Grall wrote:
> Hi Juergen,
>
> On 07/09/2022 07:28, Juergen Gross wrote:
> > On 06.09.22 19:27, Julien Grall wrote:
> > > Hi Juergen,
> > >
> > > On 05/09/2022 13:47, Juergen Gross wrote:
> > > > Extend the definition of the Xenstore migration stream to cover new
> > > > features:
> > > >
> > > > - per domain features
> > > > - extended watches (watch depth)
> > > > - per domain quota
> > > >
> > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > ---
> > > > V3:
> > > > - new patch
> > > > V4:
> > > > - add new record types instead of modifying the existing ones
> > > >    (Julien Grall)
> > > > ---
> > > >   docs/designs/xenstore-migration.md | 160 ++++++++++++++++++++++++++++-
> > > >   1 file changed, 155 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/docs/designs/xenstore-migration.md 
> > > > b/docs/designs/xenstore-migration.md
> > > > index efa526f420..c70505c43a 100644
> > > > --- a/docs/designs/xenstore-migration.md
> > > > +++ b/docs/designs/xenstore-migration.md
> > > > +|                |                                              |
> > > > +| `quota-names`  | 0 delimited strings of the quota names in    |
> > > > +|                | the same sequence as the `quota-val` values. |
> > > > +
> > > > +
> > > > +Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
> > > > +and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
> > > > +recognized by the receiving side must be ignored.
> ... this is seem to directly conflict with this sentence as to me "ignore" means 
> drop. What you want is for Xenstore to optionally preserve.
I'll rephrase that.

> Also, I think what you wrote above would be helpful in the commit message. It 
> gives some insights for future reader on how the stream was designed.
Okay.

> > > > +
> > > > +\pagebreak
> > > > +
> > > > +### DOMAIN_DATA
> > > > +
> > > > +This record is optional and can be present once for each domain.
> > > > +
> > > > +
> > > > +```
> > > > +    0       1       2       3     octet
> > > > ++-------+-------+-------+-------+
> > > > +| domain-id     | n-quota       |
> > > > ++---------------+---------------+
> > > > +| features                      |
> > > > ++-------------------------------+
> > > > +| quota-val 1                   |
> > > > ++-------------------------------+
> > > > +...
> > > > ++-------------------------------+
> > > > +| quota-val N                   |
> > > > ++-------------------------------+
> > > > +| quota-names
> > > > +...
> > > > +```
> > > > +
> > > > +
> > > > +| Field          | Description                                  |
> > > > +|----------------|----------------------------------------------|
> > > > +| `domain-id`    | The domain-id of the domain this record      |
> > > > +|                | belongs to.                                  |
> > > > +|                |                                              |
> > > > +| `n-quota`      | Number of quota values.                      |
> > > > +|                |                                              |
> > > > +| `features`     | Value of the feature field visible by the    |
> > > > +|                | guest at offset 2064 of the ring page.       |
> > > > +|                | Aligned to the next 4 octet boundary.        |
> > > Stale sentence?
> > Oh yes, a survivor of V3.
> >
> > > > +|                | Only valid for version 2 and later.          |
> > > Can you mention explicitly whether the field will unknown or 0 for version 1?
> > We have the general note "padding octets here and in all subsequent format
> > specifications must be written as zero". I think this should suffice.
> I don't view this field as padding because it has a meaning. So I argue it is 
> not cover by the sentence.
I'll rephrase that to: "padding octets or fields not valid in the used version
here and in all ..."

> Therefore I would add "Otherwise, the field is unknown/0" (pick the one you 
> prefer between 0 and unknown).
> > > > +|                |                                              |
> > > > +| `quota-val`    | Quota values, a value of 0 has the semantics |
> > > > +|                | "unlimited".                                 |
> > > > +|                |                                              |
> > > > +| `quota-names`  | 0 delimited strings of the quota names in    |
> > > > +|                | the same sequence as the `quota-val` values. |
> > > > +
> > > > +Allowed quota names are those explicitly named in [2] for the `GET_QUOTA`
> > > > +and `SET_QUOTA` commands, plus implementation specific ones. Quota names not
> > > > +recognized by the receiving side must be ignored.
> > > > +
> > > > +\pagebreak
> > > > +
> > > > +### WATCH_DATA_EXTENDED
> > > NIT: I think it would be more logical if this is defined right next after 
> > > WATCH_DATA.
> > I was following the record type numbering, but I can move this record
> > description up if you like that better.
> >
> > > > +
> > > > +The image format will contain either a `WATCH_DATA` or a `WATCH_DATA_EXTENDED`
> > > > +record for each watch registered by a connection for which there is
> > > > +`CONNECTION_DATA` record previously present. The `WATCH_DATA_EXTENDED` record
> > > > +type is valid only in version 2 and later.
> > > > +
> > > > +```
> > > > +    0       1       2       3    octet
> > > > ++-------+-------+-------+-------+
> > > > +| conn-id                       |
> > > > ++---------------+---------------+
> > > > +| wpath-len     | token-len     |
> > > > ++---------------+---------------+
> > > > +| depth         |               |
> > > > ++---------------+---------------+
> > > It is not clear what would be the value of octet 2-3. Is it RES0 or UNKNOWN?
> > I don't understand the question. conn-id is a 4-byte item.
> I was referring to the blank after 'depth'. In other record, we use 'pad'.
Okay, will add "pad" to it.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature