[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-keyrings
Subject:    Re: [PATCH 3/4] block: sed-opal: keyring support for SED Opal keys
From:       Christoph Hellwig <hch () infradead ! org>
Date:       2022-07-20 7:49:05
Message-ID: YtezcSA4c4cccQZQ () infradead ! org
[Download RAW message or body]

I don't know the keyring code at all, so just some nitpicks here:

> +	kref = keyring_search(make_key_ref(sed_opal_keyring, true),
> +			      &key_type_user,
> +			      key_name,
> +			      true);
> +
> +	if (IS_ERR(kref)) {
> +		ret = PTR_ERR(kref);

Just return directly here instead of indenting the main path.  Also the
parameter list and empty line here look odd.  Why not:

	kref = keyring_search(make_key_ref(sed_opal_keyring, true),
			      &key_type_user, key_name, true);
	if (IS_ERR(kref))
		return PTR_ERR(kref);

?

>  	ret = execute_steps(dev, pw_steps, ARRAY_SIZE(pw_steps));
>  	mutex_unlock(&dev->dev_lock);
>  
> +	if (ret == 0) {

	if (ret)
		return ret;

and avoid the indentation below.

> +	ret = update_sed_opal_key(OPAL_AUTH_KEY, init_sed_key, keylen);
> +
> +	return ret;

	return update_sed_opal_key(OPAL_AUTH_KEY, init_sed_key, keylen);

>  
> +enum opal_key_type {
> +	OPAL_INCLUDED = 0,	/* key[] is the key */
> +	OPAL_KEYRING,		/* key is in keyring */
> +};
> +
>  struct opal_key {
>  	__u8 lr;
>  	__u8 key_len;
> -	__u8 __align[6];
> +	__u8 key_type;
> +	__u8 __align[5];

Can we assume this was always zero initialized before?
[prev in list] [next in list] [prev in thread] [next in thread]