| Message ID | 20180301192935.14643-6-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On Thu, 2018-03-01 at 11:29 -0800, Bart Van Assche wrote: > Pass the VPD page number to sgio_get_vpd() such that the page needed > by the caller is queried instead of page 0x83. Fix the statement that > computes the length of the page returned by do_inq(). Fix the return > code check in the caller of sgio_get_vpd(). > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > --- Bart, thanks for the patch. Please consider rebasing your work on my previously submitted patches which have already been positively reviewed ([1], [2]): libmultipath: get_vpd_sgio: support VPD 0xc9 libmultipath: sgio_get_vpd: add page argument libmultipath: fix return code of sgio_get_vpd() Otherwise, ACK. [1] https://www.redhat.com/archives/dm-devel/2018-January/msg00241.html [2] https://www.redhat.com/archives/dm-devel/2018-January/msg00370.html Martin > libmultipath/discovery.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index d84715e15db1..780feb253797 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -836,8 +836,9 @@ detect_alua(struct path * pp, struct config > *conf) > > #define DEFAULT_SGIO_LEN 254 > > +/* Query VPD page @pg. Returns 0 upon success and -1 upon failure. > */ > static int > -sgio_get_vpd (unsigned char * buff, int maxlen, int fd) > +sgio_get_vpd (unsigned char * buff, int maxlen, int fd, int pg) > { > int len = DEFAULT_SGIO_LEN; > > @@ -846,8 +847,8 @@ sgio_get_vpd (unsigned char * buff, int maxlen, > int fd) > return -1; > } > retry: > - if (0 == do_inq(fd, 0, 1, 0x83, buff, len)) { > - len = buff[3] + (buff[2] << 8); > + if (0 == do_inq(fd, 0, 1, pg, buff, len)) { > + len = buff[3] + (buff[2] << 8) + 4; > if (len >= maxlen) > return len; > if (len > DEFAULT_SGIO_LEN) > @@ -1099,7 +1100,7 @@ get_vpd_sgio (int fd, int pg, char * str, int > maxlen) > unsigned char buff[4096]; > > memset(buff, 0x0, 4096); > - if (sgio_get_vpd(buff, 4096, fd) <= 0) { > + if (sgio_get_vpd(buff, 4096, fd, pg) < 0) { > condlog(3, "failed to issue vpd inquiry for pg%02x", > pg); > return -errno;
On Mon, 2018-03-05 at 17:53 +0100, Martin Wilck wrote: > On Thu, 2018-03-01 at 11:29 -0800, Bart Van Assche wrote: > > Pass the VPD page number to sgio_get_vpd() such that the page > > needed > > by the caller is queried instead of page 0x83. Fix the statement > > that > > computes the length of the page returned by do_inq(). Fix the > > return > > code check in the caller of sgio_get_vpd(). > > > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > > --- > > Bart, thanks for the patch. Please consider rebasing your work on my > previously submitted patches which have already been positively > reviewed ([1], [2]): > > libmultipath: get_vpd_sgio: support VPD 0xc9 > libmultipath: sgio_get_vpd: add page argument > libmultipath: fix return code of sgio_get_vpd() > > Otherwise, ACK. Sorry, I found a glitch. With the change of the "len" calculation and the return value check, we need to make sure sufficient data has been read: @@ -1100,13 +1101,19 @@ get_vpd_sgio (int fd, int pg, char * str, int maxlen) unsigned char buff[4096]; memset(buff, 0x0, 4096); - if (sgio_get_vpd(buff, 4096, fd, pg) <= 0) { + buff_len = sgio_get_vpd(buff, 4096, fd, pg); + if (buff_len < 0) { condlog(3, "failed to issue vpd inquiry for pg%02x", pg); - return -errno; + return errno != 0 ? -errno : -1; } - if (buff[1] != pg) { + if (buff_len < 4) { + condlog(3, "vpd pg%02x error, short read", pg); + return -ENODATA; + } + + if ( buff[1] != pg) { condlog(3, "vpd pg%02x error, invalid vpd page %02x", pg, buff[1]); return -ENODATA; As you can see I added also a small hunk that makes it explcicit that we don't return 0 on failure by accident. Regards, Martin
On Mon, 2018-03-05 at 18:09 +0100, Martin Wilck wrote: > On Mon, 2018-03-05 at 17:53 +0100, Martin Wilck wrote: > > On Thu, 2018-03-01 at 11:29 -0800, Bart Van Assche wrote: > > > Pass the VPD page number to sgio_get_vpd() such that the page > > > needed > > > by the caller is queried instead of page 0x83. Fix the statement > > > that > > > computes the length of the page returned by do_inq(). Fix the > > > return > > > code check in the caller of sgio_get_vpd(). > > > > > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > > > --- > > > > Bart, thanks for the patch. Please consider rebasing your work on > > my > > previously submitted patches which have already been positively > > reviewed ([1], [2]): > > > > libmultipath: get_vpd_sgio: support VPD 0xc9 > > libmultipath: sgio_get_vpd: add page argument > > libmultipath: fix return code of sgio_get_vpd() > > > > Otherwise, ACK. > > Sorry, I found a glitch. > > With the change of the "len" calculation and the return value check, > we > need to make sure sufficient data has been read: Replying to self - this is actually not necessary, as we're looking at the response buffer of a successful VPD INQUIRY, so we have to assume that at least the header has been read correctly. All we need to change is the return code and the respective comment, because the function actually returns the number of INQUIRY bytes if INQUIRY was successful (before my patch "libmultipath: fix return code of sgio_get_vpd()", the function behaved rather strange, returning -1 on failure, 0 on success, and len > buffer length on buffer overflow). Unless you object, I'll repost your series rebased on mine. Martin
On Mon, 2018-03-05 at 20:14 +0100, Martin Wilck wrote:
> Unless you object, I'll repost your series rebased on mine.
Hello Martin,
Before you start working on that: has your patch series already been posted
on the dm-devel mailing list?
Thanks,
Bart.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 2018-03-05 at 19:18 +0000, Bart Van Assche wrote: > On Mon, 2018-03-05 at 20:14 +0100, Martin Wilck wrote: > > Unless you object, I'll repost your series rebased on mine. > > Hello Martin, > > Before you start working on that: has your patch series already been > posted > on the dm-devel mailing list? Yes. "PATCH v2 00/20] Various multipath-tools fixes" ff. https://www.redhat.com/archives/dm-devel/2018-January/msg00219.html Regards, Martin
On Mon, 2018-03-05 at 21:47 +0100, Martin Wilck wrote: > On Mon, 2018-03-05 at 19:18 +0000, Bart Van Assche wrote: > > On Mon, 2018-03-05 at 20:14 +0100, Martin Wilck wrote: > > > Unless you object, I'll repost your series rebased on mine. > > > > Hello Martin, > > > > Before you start working on that: has your patch series already been > > posted > > on the dm-devel mailing list? > > Yes. "PATCH v2 00/20] Various multipath-tools fixes" ff. > https://www.redhat.com/archives/dm-devel/2018-January/msg00219.html Ah, thanks, but unfortunately these patches are no longer in my mailbox. I pulled these from https://github.com/openSUSE/multipath-tools. I'm fine with my patches being rebased on top of your series, whether or not the following issues get addressed: * Several patches that are on the upstream-queue branch introduce trailing whitespace. * The macro FREE_CONST() should never have been introduced. Introducing such a macro namely introduces the risk of calling free() for a string constant, something that should never happen. Have you considered to declare dynamically allocated strings, e.g. the result of strdup(), as char * instead of const char * ? I think with that change the FREE_CONST() macro definitions can be removed again. Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 2018-03-05 at 21:13 +0000, Bart Van Assche wrote: > On Mon, 2018-03-05 at 21:47 +0100, Martin Wilck wrote: > > On Mon, 2018-03-05 at 19:18 +0000, Bart Van Assche wrote: > > > On Mon, 2018-03-05 at 20:14 +0100, Martin Wilck wrote: > > > > Unless you object, I'll repost your series rebased on mine. > > > > > > Hello Martin, > > > > > > Before you start working on that: has your patch series already > > > been > > > posted > > > on the dm-devel mailing list? > > > > Yes. "PATCH v2 00/20] Various multipath-tools fixes" ff. > > https://www.redhat.com/archives/dm-devel/2018-January/msg00219.html > > Ah, thanks, but unfortunately these patches are no longer in my > mailbox. I > pulled these from https://github.com/openSUSE/multipath-tools. I'm > fine with > my patches being rebased on top of your series, whether or not the > following > issues get addressed: > * Several patches that are on the upstream-queue branch introduce > trailing > whitespace. > * The macro FREE_CONST() should never have been introduced. > Introducing such > a macro namely introduces the risk of calling free() for a string > constant, > something that should never happen. Have you considered to declare > dynamically allocated strings, e.g. the result of strdup(), as char > * > instead of const char * ? I think with that change the FREE_CONST() > macro > definitions can be removed again. Another philosophical issue :-) I disagree with you here. The name FREE_CONST is obvious enough that people should think twice before using it. I have indeed considered what you propse, but I like being able to use "const char*" when I work with strings that shouldn't be changed. An accidental write to a const location is much easier to overlook than a FREE_CONST(). Freeing a "const char*" isn't always wrong, just if the pointer was passed in from elsewhere. This issue is philosophical but not religious to me, so if you and maybe others are offended so much by FREE_CONST, I may ditch it again. Regards, Martin
On Tue, 2018-03-06 at 00:24 +0100, Martin Wilck wrote: > On Mon, 2018-03-05 at 21:13 +0000, Bart Van Assche wrote: > > Yes. "PATCH v2 00/20] Various multipath-tools fixes" ff. > > > https://www.redhat.com/archives/dm-devel/2018-January/msg00219.ht > > > ml > > > > Ah, thanks, but unfortunately these patches are no longer in my > > mailbox. I > > pulled these from https://github.com/openSUSE/multipath-tools. I'm > > fine with > > my patches being rebased on top of your series, whether or not the > > following > > issues get addressed: I pushed my latest state, including my rebase of your patches, to https://github.com/openSUSE/multipath-tools/commits/upstream-queue. I plan to merge also Ben's and Chongyun's late submissions, but I didn't get around to it yet. Regards, Martin
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index d84715e15db1..780feb253797 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -836,8 +836,9 @@ detect_alua(struct path * pp, struct config *conf) #define DEFAULT_SGIO_LEN 254 +/* Query VPD page @pg. Returns 0 upon success and -1 upon failure. */ static int -sgio_get_vpd (unsigned char * buff, int maxlen, int fd) +sgio_get_vpd (unsigned char * buff, int maxlen, int fd, int pg) { int len = DEFAULT_SGIO_LEN; @@ -846,8 +847,8 @@ sgio_get_vpd (unsigned char * buff, int maxlen, int fd) return -1; } retry: - if (0 == do_inq(fd, 0, 1, 0x83, buff, len)) { - len = buff[3] + (buff[2] << 8); + if (0 == do_inq(fd, 0, 1, pg, buff, len)) { + len = buff[3] + (buff[2] << 8) + 4; if (len >= maxlen) return len; if (len > DEFAULT_SGIO_LEN) @@ -1099,7 +1100,7 @@ get_vpd_sgio (int fd, int pg, char * str, int maxlen) unsigned char buff[4096]; memset(buff, 0x0, 4096); - if (sgio_get_vpd(buff, 4096, fd) <= 0) { + if (sgio_get_vpd(buff, 4096, fd, pg) < 0) { condlog(3, "failed to issue vpd inquiry for pg%02x", pg); return -errno;
Pass the VPD page number to sgio_get_vpd() such that the page needed by the caller is queried instead of page 0x83. Fix the statement that computes the length of the page returned by do_inq(). Fix the return code check in the caller of sgio_get_vpd(). Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> --- libmultipath/discovery.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)