Re: [Xen-devel] [PATCH v8 2/8] libxl: introduce a new structure to represent static shared memory regions

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Stefano Stabellini writes ("[PATCH v8 2/8] libxl: introduce a new structure to 
represent static shared memory regions"):
> From: Zhongze Liu <blackskygg@xxxxxxxxx>
> 
> Author: Zhongze Liu <blackskygg@xxxxxxxxx>
> 
> Add a new structure to the IDL family to represent static shared memory 
> regions
> as proposed in the proposal "Allow setting up shared memory areas between VMs
> from xl config file" (see [1]).
> 
> And deleted some trailing white spaces.

Can you please not add unrelated changes, even if they are only white
space changes ?  You can put them in a pre-patch and if you do that
please put my ack on the pre-patch :-).

> +libxl_sshm_cachepolicy = Enumeration("sshm_cachepolicy", [
> +    (-1, "UNKNOWN"),
> +    (0,  "ARM_NORMAL"),  # ARM policies should be < 32
> +    (32,  "X86_NORMAL"), # X86 policies should be >= 32
> +    ], init_val = "LIBXL_SSHM_CACHE_POLICY_UNKNOWN")

What ?  Why do these need separating like that ?  This is quite odd.

> +libxl_sshm_prot = Enumeration("sshm_prot", [
> +    (-1, "UNKNOWN"),
> +    (3,  "RW"),
> +    ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")

Why 3 ?

> +libxl_sshm_role = Enumeration("sshm_role", [
> +    (-1, "UNKNOWN"),
> +    (0,  "MASTER"),
> +    (1,  "SLAVE"),
> +    ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")

See my comments on the docs patch about the master/slave terminology.

Wouldn't it be better for the UNKNOWN values to be 0 ?

That would eliminate a class of possible bugs (although we do try to
avoid them anyway) where the variables are not initialised.  It would
also avoid all of these extra init_val settings:

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel