~alpine/aports

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
1

[alpine-aports] [PATCH] main/open-iscsi: musl fix for error handling. fixes bug #4802

Jann - Ove Risvik <jann.ove@usaklig.com>
Details
Message ID
<20160708015558.1578-1-jann.ove@usaklig.com>
Sender timestamp
1467942958
DKIM signature
missing
Download raw message
Patch: +28 -4
Fixes bug #4802: http://bugs.alpinelinux.org/issues/4802

Seems like iscsiadm relies on checking optopt for checking if there's an error, instead of checking whether opterr is set or if getopt returns '?' and then using optopt to retrieve the option that it didn't recognize.

For some reason musl sets optopt to the last option parsed even if it was valid. Is musl supposed to do that? glibc doesn't behave this way. The posix standard for getopt also reads like optopt is only supposed to be set when there's an error.

(Only when I was looking for what to put in the email subject line, after I figured it all out did I notice that Brian Angus already submitted a fix earlier. Figured I'd sent this one anyway; that fix removes error output when presented with actual unrecognised arguments.)
---
 main/open-iscsi/APKBUILD         |  8 ++++----
 main/open-iscsi/musl-fixes.patch | 24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/main/open-iscsi/APKBUILD b/main/open-iscsi/APKBUILD
index 6418d39..47194e3 100644
--- a/main/open-iscsi/APKBUILD
+++ b/main/open-iscsi/APKBUILD
@@ -2,7 +2,7 @@
pkgname=open-iscsi
pkgver=2.0.873
_realver=${pkgver%.*}-${pkgver##*.}
pkgrel=3
pkgrel=4
pkgdesc="High performance, transport independent, multi-platform iSCSI initiator"
url="http://www.open-iscsi.org"
arch="all"
@@ -48,14 +48,14 @@ package() {
}

md5sums="8b8316d7c9469149a6cc6234478347f7  open-iscsi-2.0-873.tar.gz
080961aef6eb9d0e8e5f65cf95411225  musl-fixes.patch
3b5e052956ba89b011b3383fe599de83  musl-fixes.patch
c6a0c15c0c21b13915179fb7e0cf0003  iscsid.initd
b762b687d4628791b4362df22cf22d34  iscsid.confd"
sha256sums="7dd9f2f97da417560349a8da44ea4fcfe98bfd5ef284240a2cc4ff8e88ac7cd9  open-iscsi-2.0-873.tar.gz
44acaab8123abb8a205732baba11e1c70bcf828dfd3ac5c42c475ee85b433507  musl-fixes.patch
6f55aa7c52c5bdefe67a8092b8d2c5a77536bd3b1b9f347fafa1f2cda94d9cd4  musl-fixes.patch
38edede472f478ce01f40e3557c315de3f3ecf1d0c0dbab2883517840a7186b5  iscsid.initd
673bf4744efc3276d372587c996270821d39dcdc0bf27a13691ea6b0e814b6d0  iscsid.confd"
sha512sums="4e67116cb7dd49381c9279645e5a661f05596ae6be3b832772089828b3764ca2d04b5dea1bcc337071efb52c3c75a6fb943136c659ee59500f3a198ed0dcea6b  open-iscsi-2.0-873.tar.gz
d40f6f14d848f2d8a2fdb11672be9b9147b71a72ac06b33f771bbe80e5fa4b5d92405df7f8978e8cd1779820e9797473bd55fc07b4f49d32c5279ac0fb39c93f  musl-fixes.patch
a51b81d62b179ff17b3ade09873113ed16b207bcccdd02cd86c76911d4b890d7ff9e1103ec3168af78560d606910374afaa04fa3356fa9e9ad6b7142a69e06e6  musl-fixes.patch
e16d0abf117c0c282e98abb14893923609dc6078f770facd0578ad72ce6e3fc7b9c84a39628c1246d955ba6bb204fb902bcba6d5959ac755fee7e2a85da181df  iscsid.initd
075bb9cb783be7ccbc799947e0e042b85310d40b3045141dc1be40ca84ed1cc0e1e54559df501c512c179e28375314b27a03c15d9a6d4b1cabd428b2279985d3  iscsid.confd"
diff --git a/main/open-iscsi/musl-fixes.patch b/main/open-iscsi/musl-fixes.patch
index 35f11e1..910b485 100644
--- a/main/open-iscsi/musl-fixes.patch
+++ b/main/open-iscsi/musl-fixes.patch
@@ -29,3 +29,27 @@
 #include <dirent.h>
 #include <limits.h>
 #include <sys/stat.h>
--- ./usr/iscsiadm.c.orig
+++ ./usr/iscsiadm.c
@@ -2553,7 +2553,10 @@ main(int argc, char **argv)
 			return 0;
 		case 'h':
 			usage(0);
-		}
+
+		case '?':
+			log_error("unrecognized character '%c'", optopt);
+		}	
 
 		if (name && value) {
 			param = idbm_alloc_user_param(name, value);
@@ -2568,8 +2571,7 @@ main(int argc, char **argv)
 		}
 	}
 
-	if (optopt) {
-		log_error("unrecognized character '%c'", optopt);
+	if (opterr) {
 		rc = ISCSI_ERR_INVAL;
 		goto free_ifaces;
 	}
-- 
2.8.3



---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---
Natanael Copa <ncopa@alpinelinux.org>
Details
Message ID
<20160811215356.31e47f57@ncopa-desktop.copa.dup.pw>
In-Reply-To
<20160708015558.1578-1-jann.ove@usaklig.com> (view parent)
Sender timestamp
1470945236
DKIM signature
missing
Download raw message
On Fri,  8 Jul 2016 03:55:58 +0200
Jann - Ove Risvik <jann.ove@usaklig.com> wrote:

> Fixes bug #4802: http://bugs.alpinelinux.org/issues/4802
> 
> Seems like iscsiadm relies on checking optopt for checking if there's an error, instead of checking whether opterr is set or if getopt returns '?' and then using optopt to retrieve the option that it didn't recognize.
> 
> For some reason musl sets optopt to the last option parsed even if it was valid. Is musl supposed to do that? glibc doesn't behave this way. The posix standard for getopt also reads like optopt is only supposed to be set when there's an error.
> 
> (Only when I was looking for what to put in the email subject line, after I figured it all out did I notice that Brian Angus already submitted a fix earlier. Figured I'd sent this one anyway; that fix removes error output when presented with actual unrecognised arguments.)


Thank you very much for your your analyze! Good job!

I mentioned it to the musl devs and it might make sense to fix this in
musl in this case. I think I'll still add a workaround to open-iscsi.

> ---
>  main/open-iscsi/APKBUILD         |  8 ++++----
>  main/open-iscsi/musl-fixes.patch | 24 ++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 4 deletions(-)

...

> diff --git a/main/open-iscsi/musl-fixes.patch b/main/open-iscsi/musl-fixes.patch
> index 35f11e1..910b485 100644
> --- a/main/open-iscsi/musl-fixes.patch
> +++ b/main/open-iscsi/musl-fixes.patch
> @@ -29,3 +29,27 @@
>   #include <dirent.h>
>   #include <limits.h>
>   #include <sys/stat.h>
> +--- ./usr/iscsiadm.c.orig
> ++++ ./usr/iscsiadm.c
> +@@ -2553,7 +2553,10 @@ main(int argc, char **argv)
> + 			return 0;
> + 		case 'h':
> + 			usage(0);
> +-		}
> ++
> ++		case '?':
> ++			log_error("unrecognized character '%c'", optopt);
> ++		}	
> + 
> + 		if (name && value) {
> + 			param = idbm_alloc_user_param(name, value);
> +@@ -2568,8 +2571,7 @@ main(int argc, char **argv)
> + 		}
> + 	}
> + 
> +-	if (optopt) {
> +-		log_error("unrecognized character '%c'", optopt);
> ++	if (opterr) {
> + 		rc = ISCSI_ERR_INVAL;
> + 		goto free_ifaces;
> + 	}


As you noticed you cannot use opterr this way.

-nc


---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---
Reply to thread Export thread (mbox)