Message ID | 1549054223-12220-4-git-send-email-zub@linux.fjfi.cvut.cz (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | block: sed-opal: support shadow MBR done flag and write | expand |
On Fri, Feb 01, 2019 at 09:50:10PM +0100, David Kozub wrote: > From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de> > > All add_token_* functions have a common set of conditions that have to > be checked. Use a common function for those checks in order to avoid > different behaviour as well as code duplication. > > Co-authored-by: David Kozub <zub@linux.fjfi.cvut.cz> > Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de> > Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz> > Reviewed-by: Scott Bauer <sbauer@plzdonthack.me> > --- > block/sed-opal.c | 30 +++++++++++++++++++++--------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/block/sed-opal.c b/block/sed-opal.c > index 5c123a5b4ab1..980705681806 100644 > --- a/block/sed-opal.c > +++ b/block/sed-opal.c > @@ -510,15 +510,29 @@ static int opal_discovery0(struct opal_dev *dev, void *data) > return opal_discovery0_end(dev); > } > > -static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok) > +static size_t remaining_size(struct opal_dev *cmd) > +{ > + return IO_BUFFER_LENGTH - cmd->pos; > +} This function seem a little pointless to me, at least as of this patch where it only has a single user just below. Otherwise this looks good to me: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, 4 Feb 2019, Christoph Hellwig wrote: > On Fri, Feb 01, 2019 at 09:50:10PM +0100, David Kozub wrote: >> From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de> >> >> All add_token_* functions have a common set of conditions that have to >> be checked. Use a common function for those checks in order to avoid >> different behaviour as well as code duplication. >> >> Co-authored-by: David Kozub <zub@linux.fjfi.cvut.cz> >> Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de> >> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz> >> Reviewed-by: Scott Bauer <sbauer@plzdonthack.me> >> --- >> block/sed-opal.c | 30 +++++++++++++++++++++--------- >> 1 file changed, 21 insertions(+), 9 deletions(-) >> >> diff --git a/block/sed-opal.c b/block/sed-opal.c >> index 5c123a5b4ab1..980705681806 100644 >> --- a/block/sed-opal.c >> +++ b/block/sed-opal.c >> @@ -510,15 +510,29 @@ static int opal_discovery0(struct opal_dev *dev, void *data) >> return opal_discovery0_end(dev); >> } >> >> -static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok) >> +static size_t remaining_size(struct opal_dev *cmd) >> +{ >> + return IO_BUFFER_LENGTH - cmd->pos; >> +} > > This function seem a little pointless to me, at least as of this patch > where it only has a single user just below. It is eventually used for the second time in 11/16 block: sed-opal: ioctl for writing to shadow mbr. If you feel strongly about this I can exclude it from this commit and introduce it in 11/16 (where it then will called from here and from write_shadow_mbr). Best regards, David
On Mon, Feb 04, 2019 at 10:07:09PM +0100, David Kozub wrote: > It is eventually used for the second time in 11/16 block: sed-opal: ioctl > for writing to shadow mbr. > > If you feel strongly about this I can exclude it from this commit and > introduce it in 11/16 (where it then will called from here and from > write_shadow_mbr). No problem, just keep it.
On Mon, 2019-02-04 at 22:07 +0100, David Kozub wrote: > On Mon, 4 Feb 2019, Christoph Hellwig wrote: > > > On Fri, Feb 01, 2019 at 09:50:10PM +0100, David Kozub wrote: > > > From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de> > > > > > > All add_token_* functions have a common set of conditions that have to > > > be checked. Use a common function for those checks in order to avoid > > > different behaviour as well as code duplication. > > > > > > Co-authored-by: David Kozub <zub@linux.fjfi.cvut.cz> > > > Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de> > > > Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz> > > > Reviewed-by: Scott Bauer <sbauer@plzdonthack.me> > > > --- > > > block/sed-opal.c | 30 +++++++++++++++++++++--------- > > > 1 file changed, 21 insertions(+), 9 deletions(-) > > > > > > diff --git a/block/sed-opal.c b/block/sed-opal.c > > > index 5c123a5b4ab1..980705681806 100644 > > > --- a/block/sed-opal.c > > > +++ b/block/sed-opal.c > > > @@ -510,15 +510,29 @@ static int opal_discovery0(struct opal_dev *dev, void *data) > > > return opal_discovery0_end(dev); > > > } > > > > > > -static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok) > > > +static size_t remaining_size(struct opal_dev *cmd) > > > +{ > > > + return IO_BUFFER_LENGTH - cmd->pos; > > > +} > > > > This function seem a little pointless to me, at least as of this patch > > where it only has a single user just below. > > It is eventually used for the second time in 11/16 block: sed-opal: ioctl > for writing to shadow mbr. > > If you feel strongly about this I can exclude it from this commit and > introduce it in 11/16 (where it then will called from here and from > write_shadow_mbr). > > Best regards, > David I'd prefer this option where we refactor later Otherwise looks good Reviewed-by Jon Derrick <jonathan.derrick@intel.com>
diff --git a/block/sed-opal.c b/block/sed-opal.c index 5c123a5b4ab1..980705681806 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -510,15 +510,29 @@ static int opal_discovery0(struct opal_dev *dev, void *data) return opal_discovery0_end(dev); } -static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok) +static size_t remaining_size(struct opal_dev *cmd) +{ + return IO_BUFFER_LENGTH - cmd->pos; +} + +static bool can_add(int *err, struct opal_dev *cmd, size_t len) { if (*err) - return; - if (cmd->pos >= IO_BUFFER_LENGTH - 1) { - pr_debug("Error adding u8: end of buffer.\n"); + return false; + + if (remaining_size(cmd) < len) { + pr_debug("Error adding %zu bytes: end of buffer.\n", len); *err = -ERANGE; - return; + return false; } + + return true; +} + +static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok) +{ + if (!can_add(err, cmd, 1)) + return; cmd->cmd[cmd->pos++] = tok; } @@ -563,9 +577,8 @@ static void add_token_u64(int *err, struct opal_dev *cmd, u64 number) msb = fls64(number); len = DIV_ROUND_UP(msb, 8); - if (cmd->pos >= IO_BUFFER_LENGTH - len - 1) { + if (!can_add(err, cmd, len + 1)) { pr_debug("Error adding u64: end of buffer.\n"); - *err = -ERANGE; return; } add_short_atom_header(cmd, false, false, len); @@ -587,9 +600,8 @@ static void add_token_bytestring(int *err, struct opal_dev *cmd, is_short_atom = false; } - if (len >= IO_BUFFER_LENGTH - cmd->pos - header_len) { + if (!can_add(err, cmd, header_len + len)) { pr_debug("Error adding bytestring: end of buffer.\n"); - *err = -ERANGE; return; }