Re: [pve-devel] [PATCH cluster 2/3] Cluster: add get_guest_config_properties

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

this and the other one existing one might also warrant a comment 
indicating that you possibly still want to parse/validate/filter the 
result before passing it along further up the stack (or that it should 
only be used for very simple keys?)

- if no vmid is passed, the result needs to be filtered by access to not 
  leak information
- if property is anything but a very simple type, validation might be 
  important (for lock it's not that bad since that is just a simple 
  enum, so a bogus/invalid value will likely just look weird, for the 
  `tags` property it's also not that bad since those are just
  simple lists of strings, but who knows what this will get used for 
  down the line ;))

also depending on whether the msg format gets changed this will need 
adapation obviously.

On March 14, 2022 10:03 am, Dominik Csapak wrote:
> akin to get_guest_config_property, but with a list of properties.
> uses the new CFS_IPC_GET_GUEST_CONFIG_PROPERTIES
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  data/PVE/Cluster.pm | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index 765fb43..c65ba17 100644
> --- a/data/PVE/Cluster.pm
> +++ b/data/PVE/Cluster.pm
> @@ -340,6 +340,27 @@ sub get_node_kv {
>      return $res;
>  }
>  
> +# properties: an array-ref of config properties you want to get, e.g., this
> +# is perfect to get multiple properties of a guest _fast_
> +# (>100 faster than manual parsing here)
> +# vmid: optional, if a valid is passed we only check that one, else return all
> +# NOTE: does *not* searches snapshot and PENDING entries sections!
> +sub get_guest_config_properties {
> +    my ($properties, $vmid) = @_;
> +
> +    die "properties required" if !defined($properties);
> +
> +    my $bindata = pack "VC", $vmid // 0, scalar(@$properties);

length of $properties actually has a limit which should maybe be checked 
here as well? I mean it is rather unlikely to be misused in practice, 
and will print a warning that the `C` wraps the value here, but..

> +    for my $property (@$properties) {

> +	die "property name cannot be longer than 254 chars\n"
> +	    if length($property) > 254;
> +	$bindata .= pack "C/Z*", $property;

like indicated in the other patch's comments, this is a rather strange 
encoding (flashbacks to ASN.1 ;)) and shouldn't be needed.

> +    }
> +    my $res = $ipcc_send_rec_json->(CFS_IPC_GET_GUEST_CONFIG_PROPERTIES, $bindata);
> +
> +    return $res;
> +}
> +
>  # property: a config property you want to get, e.g., this is perfect to get
>  # the 'lock' entry of a guest _fast_ (>100 faster than manual parsing here)
>  # vmid: optipnal, if a valid is passed we only check that one, else return all
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>