| Message ID | 20230322151604.401680-2-okozina@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sed-opal: add command to read locking range attributes | expand |
On Wed, Mar 22, 2023 at 04:16:00PM +0100, Ondrej Kozina wrote: > While adding user authority in boolean ace value > of uid OPAL_LOCKINGRANGE_ACE_WRLOCKED or > OPAL_LOCKINGRANGE_ACE_RDLOCKED, it was added twice. > > Signed-off-by: Ondrej Kozina <okozina@redhat.com> > Tested-by: Luca Boccassi <bluca@debian.org> > Tested-by: Milan Broz <gmazyland@gmail.com> > --- > block/sed-opal.c | 16 ---------------- > 1 file changed, 16 deletions(-) > > diff --git a/block/sed-opal.c b/block/sed-opal.c > index c320093c14f1..d86d3e5f5a44 100644 > --- a/block/sed-opal.c > +++ b/block/sed-opal.c > @@ -1798,22 +1798,6 @@ static int add_user_to_lr(struct opal_dev *dev, void *data) > add_token_bytestring(&err, dev, user_uid, OPAL_UID_LENGTH); > add_token_u8(&err, dev, OPAL_ENDNAME); > > - > - add_token_u8(&err, dev, OPAL_STARTNAME); > - add_token_bytestring(&err, dev, > - opaluid[OPAL_HALF_UID_AUTHORITY_OBJ_REF], > - OPAL_UID_LENGTH/2); > - add_token_bytestring(&err, dev, user_uid, OPAL_UID_LENGTH); > - add_token_u8(&err, dev, OPAL_ENDNAME); > - > - > - add_token_u8(&err, dev, OPAL_STARTNAME); > - add_token_bytestring(&err, dev, opaluid[OPAL_HALF_UID_BOOLEAN_ACE], This index only appears one time in the code. IOW, you're completely removing OPAL_HALF_UID_BOOLEAN_ACE leavig only OPAL_HALF_UID_AUTHORITY_OBJ_REF. Is that intended and if so why is OPAL_HALF_UID_BOOLEAN_ACE not needed anymore?
On 29. 03. 23 16:15, Christian Brauner wrote: > On Wed, Mar 22, 2023 at 04:16:00PM +0100, Ondrej Kozina wrote: > > This index only appears one time in the code. IOW, you're completely > removing OPAL_HALF_UID_BOOLEAN_ACE leavig only > OPAL_HALF_UID_AUTHORITY_OBJ_REF. Is that intended and if so why is > OPAL_HALF_UID_BOOLEAN_ACE not needed anymore? > It seemed redundant when only single authority is added in the set method aka { authority1, authority1, OR }: TCG Storage Architecture Core Specification, 5.1.3.3 ACE_expression "This is an alternative type where the options are either a uidref to an Authority object or one of the boolean_ACE (AND = 0 and OR = 1) options. This type is used within the AC_element list to form a postfix Boolean expression of Authorities." I add OPAL_HALF_UID_BOOLEAN_ACE when there's more than single authority added in any ACE_expression in later code.
On Wed, Mar 29, 2023 at 05:20:29PM +0200, Ondrej Kozina wrote: > It seemed redundant when only single authority is added in the set method > aka { authority1, authority1, OR }: > > TCG Storage Architecture Core Specification, 5.1.3.3 ACE_expression > > "This is an alternative type where the options are either a uidref to an > Authority object or one of the boolean_ACE (AND = 0 and OR = 1) options. > This type is used within the AC_element list to form a postfix Boolean > expression of Authorities." Can you add this information to the commit message? With that: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Mar 29, 2023 at 05:20:29PM +0200, Ondrej Kozina wrote: > On 29. 03. 23 16:15, Christian Brauner wrote: > > On Wed, Mar 22, 2023 at 04:16:00PM +0100, Ondrej Kozina wrote: > > > > This index only appears one time in the code. IOW, you're completely > > removing OPAL_HALF_UID_BOOLEAN_ACE leavig only > > OPAL_HALF_UID_AUTHORITY_OBJ_REF. Is that intended and if so why is > > OPAL_HALF_UID_BOOLEAN_ACE not needed anymore? > > > > It seemed redundant when only single authority is added in the set method > aka { authority1, authority1, OR }: > > TCG Storage Architecture Core Specification, 5.1.3.3 ACE_expression > > "This is an alternative type where the options are either a uidref to an > Authority object or one of the boolean_ACE (AND = 0 and OR = 1) options. > This type is used within the AC_element list to form a postfix Boolean > expression of Authorities." > > I add OPAL_HALF_UID_BOOLEAN_ACE when there's more than single authority > added in any ACE_expression in later code. Ok, thanks! As Christoph said, would be good to have this in the commit message. Otherwise, Acked-by: Christian Brauner <brauner@kernel.org>
diff --git a/block/sed-opal.c b/block/sed-opal.c index c320093c14f1..d86d3e5f5a44 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -1798,22 +1798,6 @@ static int add_user_to_lr(struct opal_dev *dev, void *data) add_token_bytestring(&err, dev, user_uid, OPAL_UID_LENGTH); add_token_u8(&err, dev, OPAL_ENDNAME); - - add_token_u8(&err, dev, OPAL_STARTNAME); - add_token_bytestring(&err, dev, - opaluid[OPAL_HALF_UID_AUTHORITY_OBJ_REF], - OPAL_UID_LENGTH/2); - add_token_bytestring(&err, dev, user_uid, OPAL_UID_LENGTH); - add_token_u8(&err, dev, OPAL_ENDNAME); - - - add_token_u8(&err, dev, OPAL_STARTNAME); - add_token_bytestring(&err, dev, opaluid[OPAL_HALF_UID_BOOLEAN_ACE], - OPAL_UID_LENGTH/2); - add_token_u8(&err, dev, 1); - add_token_u8(&err, dev, OPAL_ENDNAME); - - add_token_u8(&err, dev, OPAL_ENDLIST); add_token_u8(&err, dev, OPAL_ENDNAME); add_token_u8(&err, dev, OPAL_ENDLIST);