[RFC bpf-next 0/4] Error handling when map lookup isn't supported

[RFC bpf-next 0/4] Error handling when map lookup isn't supported

[Prashant Bhole]

Currently when map a lookup is failed, user space API can not make any
distinction whether given key was not found or lookup is not supported
by particular map.

In this series we modify return value of maps which do not support
lookup. Lookup on such map implementation will return -EOPNOTSUPP.
bpf() syscall with BPF_MAP_LOOKUP_ELEM command will set EOPNOTSUPP
errno. We also handle this error in bpftool to print appropriate
message.

Patch 1: adds handling of BPF_MAP_LOOKUP ELEM command of bpf syscall
such that errno will set to EOPNOTSUPP when map doesn't support lookup

Patch 2: Modifies the return value of map_lookup_elem() to EOPNOTSUPP
for maps which do not support lookup

Patch 3: Splits do_dump() in bpftool/map.c. Element printing code is
moved out into new function dump_map_elem(). This was done in order to
reduce deep indentation and accomodate further changes.

Patch 4: Changes to bpftool to do additional checking for errno when
map lookup is failed. In case of EOPNOTSUPP errno, it prints message
"lookup not supported for this map"

Prashant Bhole (4):
  bpf: error handling when map_lookup_elem isn't supported
  bpf: return EOPNOTSUPP when map lookup isn't supported
  tools/bpf: bpftool, split the function do_dump()
  tools/bpf: handle EOPNOTSUPP when map lookup is failed

 kernel/bpf/arraymap.c    |   2 +-
 kernel/bpf/sockmap.c     |   2 +-
 kernel/bpf/stackmap.c    |   2 +-
 kernel/bpf/syscall.c     |   9 +++-
 kernel/bpf/xskmap.c      |   2 +-
 tools/bpf/bpftool/main.h |   5 ++
 tools/bpf/bpftool/map.c  | 108 +++++++++++++++++++++++++++------------
 7 files changed, 90 insertions(+), 40 deletions(-)

-- 
2.17.1

[RFC bpf-next 1/4] bpf: error handling when map_lookup_elem isn't supported

[Prashant Bhole]

The error value returned by map_lookup_elem doesn't differentiate
whether lookup was failed because of invalid key or lookup is not
supported.

Lets add handling for -EOPNOTSUPP return value of map_lookup_elem()
method of map, with expectation from map's implementation that it
should return -EOPNOTSUPP if lookup is not supported.

The errno for bpf syscall for BPF_MAP_LOOKUP_ELEM command will be set
to EOPNOTSUPP if map lookup is not supported.

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 kernel/bpf/syscall.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b3c2d09bcf7a..ecb06352b5a0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -716,10 +716,15 @@ static int map_lookup_elem(union bpf_attr *attr)
 	} else {
 		rcu_read_lock();
 		ptr = map->ops->map_lookup_elem(map, key);
-		if (ptr)
+		if (IS_ERR(ptr)) {
+			err = PTR_ERR(ptr);
+		} else if (!ptr) {
+			err = -ENOENT;
+		} else {
+			err = 0;
 			memcpy(value, ptr, value_size);
+		}
 		rcu_read_unlock();
-		err = ptr ? 0 : -ENOENT;
 	}
 
 	if (err)
-- 
2.17.1

[RFC bpf-next 2/4] bpf: return EOPNOTSUPP when map lookup isn't supported

[Prashant Bhole]

Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
map types:
- BPF_MAP_TYPE_PROG_ARRAY
- BPF_MAP_TYPE_STACK_TRACE
- BPF_MAP_TYPE_XSKMAP
- BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 kernel/bpf/arraymap.c | 2 +-
 kernel/bpf/sockmap.c  | 2 +-
 kernel/bpf/stackmap.c | 2 +-
 kernel/bpf/xskmap.c   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index dded84cbe814..24583da9ffd1 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
 
 static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
 {
-	return NULL;
+	return ERR_PTR(-EOPNOTSUPP);
 }
 
 /* only called from syscall */
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 488ef9663c01..e50922a802f7 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -2076,7 +2076,7 @@ int sockmap_get_from_fd(const union bpf_attr *attr, int type,
 
 static void *sock_map_lookup(struct bpf_map *map, void *key)
 {
-	return NULL;
+	return ERR_PTR(-EOPNOTSUPP);
 }
 
 static int sock_map_update_elem(struct bpf_map *map,
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 8061a439ef18..b2ade10f7ec3 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -505,7 +505,7 @@ const struct bpf_func_proto bpf_get_stack_proto = {
 /* Called from eBPF program */
 static void *stack_map_lookup_elem(struct bpf_map *map, void *key)
 {
-	return NULL;
+	return ERR_PTR(-EOPNOTSUPP);
 }
 
 /* Called from syscall */
diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
index 9f8463afda9c..ef0b7b6ef8a5 100644
--- a/kernel/bpf/xskmap.c
+++ b/kernel/bpf/xskmap.c
@@ -154,7 +154,7 @@ void __xsk_map_flush(struct bpf_map *map)
 
 static void *xsk_map_lookup_elem(struct bpf_map *map, void *key)
 {
-	return NULL;
+	return ERR_PTR(-EOPNOTSUPP);
 }
 
 static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
-- 
2.17.1

[RFC bpf-next 3/4] tools/bpf: bpftool, split the function do_dump()

[Prashant Bhole]

do_dump() function in bpftool/map.c has deep indentations. In order
to reduce deep indent, let's move element printing code out of
do_dump() into dump_map_elem() function.

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/bpf/bpftool/map.c | 83 ++++++++++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index af8ad32fa6e9..284e12a289c0 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -655,6 +655,54 @@ static int do_show(int argc, char **argv)
 	return errno == ENOENT ? 0 : -1;
 }
 
+static int dump_map_elem(int fd, void *key, void *value,
+			 struct bpf_map_info *map_info, struct btf *btf,
+			 json_writer_t *btf_wtr)
+{
+	int num_elems = 0;
+
+	if (!bpf_map_lookup_elem(fd, key, value)) {
+		if (json_output) {
+			print_entry_json(map_info, key, value, btf);
+		} else {
+			if (btf) {
+				struct btf_dumper d = {
+					.btf = btf,
+					.jw = btf_wtr,
+					.is_plain_text = true,
+				};
+
+				do_dump_btf(&d, map_info, key, value);
+			} else {
+				print_entry_plain(map_info, key, value);
+			}
+			num_elems++;
+		}
+		goto out;
+	}
+
+	/* lookup error handling */
+	if (map_is_map_of_maps(map_info->type) ||
+	    map_is_map_of_progs(map_info->type))
+		goto out;
+
+	if (json_output) {
+		jsonw_name(json_wtr, "key");
+		print_hex_data_json(key, map_info->key_size);
+		jsonw_name(json_wtr, "value");
+		jsonw_start_object(json_wtr);
+		jsonw_string_field(json_wtr, "error",
+				   "can't lookup element");
+		jsonw_end_object(json_wtr);
+	} else {
+		p_info("can't lookup element with key: ");
+		fprint_hex(stderr, key, map_info->key_size, " ");
+		fprintf(stderr, "\n");
+	}
+out:
+	return num_elems;
+}
+
 static int do_dump(int argc, char **argv)
 {
 	struct bpf_map_info info = {};
@@ -710,40 +758,7 @@ static int do_dump(int argc, char **argv)
 				err = 0;
 			break;
 		}
-
-		if (!bpf_map_lookup_elem(fd, key, value)) {
-			if (json_output)
-				print_entry_json(&info, key, value, btf);
-			else
-				if (btf) {
-					struct btf_dumper d = {
-						.btf = btf,
-						.jw = btf_wtr,
-						.is_plain_text = true,
-					};
-
-					do_dump_btf(&d, &info, key, value);
-				} else {
-					print_entry_plain(&info, key, value);
-				}
-			num_elems++;
-		} else if (!map_is_map_of_maps(info.type) &&
-			   !map_is_map_of_progs(info.type)) {
-			if (json_output) {
-				jsonw_name(json_wtr, "key");
-				print_hex_data_json(key, info.key_size);
-				jsonw_name(json_wtr, "value");
-				jsonw_start_object(json_wtr);
-				jsonw_string_field(json_wtr, "error",
-						   "can't lookup element");
-				jsonw_end_object(json_wtr);
-			} else {
-				p_info("can't lookup element with key: ");
-				fprint_hex(stderr, key, info.key_size, " ");
-				fprintf(stderr, "\n");
-			}
-		}
-
+		num_elems += dump_map_elem(fd, key, value, &info, btf, btf_wtr);
 		prev_key = key;
 	}
 
-- 
2.17.1

[RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed

[Prashant Bhole]

Let's add a check for EOPNOTSUPP error when map lookup is failed.
Also in case map doesn't support lookup, the output of map dump is
changed from "can't lookup element" to "lookup not supported for
this map".

Patch adds function print_entry_error() function to print the error
value.

Following example dumps a map which does not support lookup.

Output before:
root# bpftool map -jp dump id 40
[
    "key": ["0x0a","0x00","0x00","0x00"
    ],
    "value": {
        "error": "can\'t lookup element"
    },
    "key": ["0x0b","0x00","0x00","0x00"
    ],
    "value": {
        "error": "can\'t lookup element"
    }
]

root# bpftool map dump id 40
can't lookup element with key:
0a 00 00 00
can't lookup element with key:
0b 00 00 00
Found 0 elements

Output after changes:
root# bpftool map dump -jp  id 45
[
    "key": ["0x0a","0x00","0x00","0x00"
    ],
    "value": {
        "error": "lookup not supported for this map"
    },
    "key": ["0x0b","0x00","0x00","0x00"
    ],
    "value": {
        "error": "lookup not supported for this map"
    }
]

root# bpftool map dump id 45
key:
0a 00 00 00
value:
lookup not supported for this map
key:
0b 00 00 00
value:
lookup not supported for this map
Found 0 elements

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/bpf/bpftool/main.h |  5 +++++
 tools/bpf/bpftool/map.c  | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 40492cdc4e53..1a8c683f949b 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -46,6 +46,11 @@
 
 #include "json_writer.h"
 
+#define ERR_CANNOT_LOOKUP \
+	"can't lookup element"
+#define ERR_LOOKUP_NOT_SUPPORTED \
+	"lookup not supported for this map"
+
 #define ptr_to_u64(ptr)	((__u64)(unsigned long)(ptr))
 
 #define NEXT_ARG()	({ argc--; argv++; if (argc < 0) usage(); })
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 284e12a289c0..2faccd2098c9 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -333,6 +333,25 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
 	jsonw_end_object(json_wtr);
 }
 
+static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
+			      char *value)
+{
+	bool single_line, break_names;
+	int value_size = strlen(value);
+
+	break_names = info->key_size > 16 || value_size > 16;
+	single_line = info->key_size + value_size <= 24 && !break_names;
+
+	printf("key:%c", break_names ? '\n' : ' ');
+	fprint_hex(stdout, key, info->key_size, " ");
+
+	printf(single_line ? "  " : "\n");
+
+	printf("value:%c%s", break_names ? '\n' : ' ', value);
+
+	printf("\n");
+}
+
 static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
 			      unsigned char *value)
 {
@@ -660,6 +679,8 @@ static int dump_map_elem(int fd, void *key, void *value,
 			 json_writer_t *btf_wtr)
 {
 	int num_elems = 0;
+	int lookup_errno;
+	char *errstr;
 
 	if (!bpf_map_lookup_elem(fd, key, value)) {
 		if (json_output) {
@@ -682,22 +703,26 @@ static int dump_map_elem(int fd, void *key, void *value,
 	}
 
 	/* lookup error handling */
+	lookup_errno = errno;
+
 	if (map_is_map_of_maps(map_info->type) ||
 	    map_is_map_of_progs(map_info->type))
 		goto out;
 
+	if (lookup_errno == EOPNOTSUPP)
+		errstr = ERR_LOOKUP_NOT_SUPPORTED;
+	else
+		errstr = ERR_CANNOT_LOOKUP;
+
 	if (json_output) {
 		jsonw_name(json_wtr, "key");
 		print_hex_data_json(key, map_info->key_size);
 		jsonw_name(json_wtr, "value");
 		jsonw_start_object(json_wtr);
-		jsonw_string_field(json_wtr, "error",
-				   "can't lookup element");
+		jsonw_string_field(json_wtr, "error", errstr);
 		jsonw_end_object(json_wtr);
 	} else {
-		p_info("can't lookup element with key: ");
-		fprint_hex(stderr, key, map_info->key_size, " ");
-		fprintf(stderr, "\n");
+		print_entry_error(map_info, key, errstr);
 	}
 out:
 	return num_elems;
-- 
2.17.1

Re: [RFC bpf-next 2/4] bpf: return EOPNOTSUPP when map lookup isn't supported

[Alexei Starovoitov]

On Wed, Sep 19, 2018 at 04:51:41PM +0900, Prashant Bhole wrote:
> Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
> map types:
> - BPF_MAP_TYPE_PROG_ARRAY
> - BPF_MAP_TYPE_STACK_TRACE
> - BPF_MAP_TYPE_XSKMAP
> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
> 
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
>  kernel/bpf/arraymap.c | 2 +-
>  kernel/bpf/sockmap.c  | 2 +-
>  kernel/bpf/stackmap.c | 2 +-
>  kernel/bpf/xskmap.c   | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index dded84cbe814..24583da9ffd1 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
>  
>  static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
>  {
> -	return NULL;
> +	return ERR_PTR(-EOPNOTSUPP);
>  }

conceptually the set looks good to me.
Please add a test to test_verifier.c to make sure
that these lookup helpers cannot be called from BPF program.
Otherwise this diff may cause crashes.

Re: [RFC bpf-next 3/4] tools/bpf: bpftool, split the function do_dump()

[Jakub Kicinski]

On Wed, 19 Sep 2018 16:51:42 +0900, Prashant Bhole wrote:
> +static int dump_map_elem(int fd, void *key, void *value,
> +			 struct bpf_map_info *map_info, struct btf *btf,
> +			 json_writer_t *btf_wtr)
> +{
> +	int num_elems = 0;
> +
> +	if (!bpf_map_lookup_elem(fd, key, value)) {
> +		if (json_output) {
> +			print_entry_json(map_info, key, value, btf);
> +		} else {
> +			if (btf) {
> +				struct btf_dumper d = {
> +					.btf = btf,
> +					.jw = btf_wtr,
> +					.is_plain_text = true,
> +				};
> +
> +				do_dump_btf(&d, map_info, key, value);
> +			} else {
> +				print_entry_plain(map_info, key, value);
> +			}
> +			num_elems++;
> +		}
> +		goto out;
> +	}
> +
> +	/* lookup error handling */
> +	if (map_is_map_of_maps(map_info->type) ||
> +	    map_is_map_of_progs(map_info->type))
> +		goto out;
> +

nit: why not just return?  the goto seems to only do a return anyway,
     is this suggested by some coding style?  Is it to help the
     compiler?  I see people do this from time to time..

[...]

> +out:
> +	return num_elems;

Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed

[Jakub Kicinski]

On Wed, 19 Sep 2018 16:51:43 +0900, Prashant Bhole wrote:
> Let's add a check for EOPNOTSUPP error when map lookup is failed.
> Also in case map doesn't support lookup, the output of map dump is
> changed from "can't lookup element" to "lookup not supported for
> this map".
> 
> Patch adds function print_entry_error() function to print the error
> value.
> 
> Following example dumps a map which does not support lookup.
> 
> Output before:
> root# bpftool map -jp dump id 40
> [
>     "key": ["0x0a","0x00","0x00","0x00"
>     ],
>     "value": {
>         "error": "can\'t lookup element"
>     },
>     "key": ["0x0b","0x00","0x00","0x00"
>     ],
>     "value": {
>         "error": "can\'t lookup element"
>     }
> ]
> 
> root# bpftool map dump id 40
> can't lookup element with key:
> 0a 00 00 00
> can't lookup element with key:
> 0b 00 00 00
> Found 0 elements
> 
> Output after changes:
> root# bpftool map dump -jp  id 45
> [
>     "key": ["0x0a","0x00","0x00","0x00"
>     ],
>     "value": {
>         "error": "lookup not supported for this map"
>     },
>     "key": ["0x0b","0x00","0x00","0x00"
>     ],
>     "value": {
>         "error": "lookup not supported for this map"
>     }
> ]
> 
> root# bpftool map dump id 45
> key:
> 0a 00 00 00
> value:
> lookup not supported for this map
> key:
> 0b 00 00 00
> value:
> lookup not supported for this map
> Found 0 elements

Nice improvement, thanks for the changes!  I wonder what your thoughts
would be on just printing some form of "lookup not supported for this
map" only once?  It seems slightly like repeated information - if
lookup is not supported for one key it likely won't be for other keys
too, so we could shorten the output.  Would that make sense?

> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
>  tools/bpf/bpftool/main.h |  5 +++++
>  tools/bpf/bpftool/map.c  | 35 ++++++++++++++++++++++++++++++-----
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 40492cdc4e53..1a8c683f949b 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -46,6 +46,11 @@
>  
>  #include "json_writer.h"
>  
> +#define ERR_CANNOT_LOOKUP \
> +	"can't lookup element"
> +#define ERR_LOOKUP_NOT_SUPPORTED \
> +	"lookup not supported for this map"

Do we need these?  Are we going to reused them in more parts of the
code?

>  #define ptr_to_u64(ptr)	((__u64)(unsigned long)(ptr))
>  
>  #define NEXT_ARG()	({ argc--; argv++; if (argc < 0) usage(); })
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 284e12a289c0..2faccd2098c9 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -333,6 +333,25 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>  	jsonw_end_object(json_wtr);
>  }
>  
> +static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> +			      char *value)
> +{
> +	bool single_line, break_names;
> +	int value_size = strlen(value);

nit: order variables declaration lines to shortest, please.

> +
> +	break_names = info->key_size > 16 || value_size > 16;
> +	single_line = info->key_size + value_size <= 24 && !break_names;
> +
> +	printf("key:%c", break_names ? '\n' : ' ');
> +	fprint_hex(stdout, key, info->key_size, " ");
> +
> +	printf(single_line ? "  " : "\n");
> +
> +	printf("value:%c%s", break_names ? '\n' : ' ', value);
> +
> +	printf("\n");
> +}
> +
>  static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>  			      unsigned char *value)
>  {
> @@ -660,6 +679,8 @@ static int dump_map_elem(int fd, void *key, void *value,
>  			 json_writer_t *btf_wtr)
>  {
>  	int num_elems = 0;
> +	int lookup_errno;
> +	char *errstr;

nit: const char?

>  
>  	if (!bpf_map_lookup_elem(fd, key, value)) {
>  		if (json_output) {

Re: [RFC bpf-next 2/4] bpf: return EOPNOTSUPP when map lookup isn't supported

[Mauricio Vasquez]

On 09/19/2018 10:14 AM, Alexei Starovoitov wrote:
> On Wed, Sep 19, 2018 at 04:51:41PM +0900, Prashant Bhole wrote:
>> Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
>> map types:
>> - BPF_MAP_TYPE_PROG_ARRAY
>> - BPF_MAP_TYPE_STACK_TRACE
>> - BPF_MAP_TYPE_XSKMAP
>> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
>>
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> ---
>>   kernel/bpf/arraymap.c | 2 +-
>>   kernel/bpf/sockmap.c  | 2 +-
>>   kernel/bpf/stackmap.c | 2 +-
>>   kernel/bpf/xskmap.c   | 2 +-
>>   4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index dded84cbe814..24583da9ffd1 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
>>   
>>   static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
>>   {
>> -	return NULL;
>> +	return ERR_PTR(-EOPNOTSUPP);
>>   }
> conceptually the set looks good to me.
> Please add a test to test_verifier.c to make sure
> that these lookup helpers cannot be called from BPF program.
> Otherwise this diff may cause crashes.
>
>
I think we can remove all these stub functions that return EOPNOTSUPP or 
EINVAL and return the error in syscall.c if ops->map_[update, delete, 
lookup, ...] is null.
This will require to extend (and test) the verifier to guarantee that 
those helpers are not called in wrong maps, for example 
map_delete_element in array like maps.

Would it make sense?

Re: [RFC bpf-next 2/4] bpf: return EOPNOTSUPP when map lookup isn't supported

[Prashant Bhole]

On 9/20/2018 3:40 AM, Mauricio Vasquez wrote:
> 
> 
> On 09/19/2018 10:14 AM, Alexei Starovoitov wrote:
>> On Wed, Sep 19, 2018 at 04:51:41PM +0900, Prashant Bhole wrote:
>>> Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
>>> map types:
>>> - BPF_MAP_TYPE_PROG_ARRAY
>>> - BPF_MAP_TYPE_STACK_TRACE
>>> - BPF_MAP_TYPE_XSKMAP
>>> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
>>>
>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>> ---
>>>   kernel/bpf/arraymap.c | 2 +-
>>>   kernel/bpf/sockmap.c  | 2 +-
>>>   kernel/bpf/stackmap.c | 2 +-
>>>   kernel/bpf/xskmap.c   | 2 +-
>>>   4 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>>> index dded84cbe814..24583da9ffd1 100644
>>> --- a/kernel/bpf/arraymap.c
>>> +++ b/kernel/bpf/arraymap.c
>>> @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
>>>   static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
>>>   {
>>> -    return NULL;
>>> +    return ERR_PTR(-EOPNOTSUPP);
>>>   }
>> conceptually the set looks good to me.
>> Please add a test to test_verifier.c to make sure
>> that these lookup helpers cannot be called from BPF program.
>> Otherwise this diff may cause crashes.
>>
>>
> I think we can remove all these stub functions that return EOPNOTSUPP or 
> EINVAL and return the error in syscall.c if ops->map_[update, delete, 
> lookup, ...] is null.
> This will require to extend (and test) the verifier to guarantee that 
> those helpers are not called in wrong maps, for example 
> map_delete_element in array like maps.
> 
> Would it make sense?

Thanks for review and suggestion.

I had thought about this way too (except adding restrictions in the 
verifier). There is no strong reason for choosing current implementation.

I thought there must be some reason that those methods are implemented 
and just returning NULL. Also there are no NULL checks for 
map_lookup_elem stub. So I decided to simply change the return value.
If some more people agree with removing stub function idea, I will do it.

-Prashant

Re: [RFC bpf-next 2/4] bpf: return EOPNOTSUPP when map lookup isn't supported

[Prashant Bhole]

On 9/20/2018 12:14 AM, Alexei Starovoitov wrote:
> On Wed, Sep 19, 2018 at 04:51:41PM +0900, Prashant Bhole wrote:
>> Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
>> map types:
>> - BPF_MAP_TYPE_PROG_ARRAY
>> - BPF_MAP_TYPE_STACK_TRACE
>> - BPF_MAP_TYPE_XSKMAP
>> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
>>
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> ---
>>   kernel/bpf/arraymap.c | 2 +-
>>   kernel/bpf/sockmap.c  | 2 +-
>>   kernel/bpf/stackmap.c | 2 +-
>>   kernel/bpf/xskmap.c   | 2 +-
>>   4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index dded84cbe814..24583da9ffd1 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
>>   
>>   static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
>>   {
>> -	return NULL;
>> +	return ERR_PTR(-EOPNOTSUPP);
>>   }
> 
> conceptually the set looks good to me.
> Please add a test to test_verifier.c to make sure
> that these lookup helpers cannot be called from BPF program.
> Otherwise this diff may cause crashes.

Thanks for reviewing.
Is the verifier change below sufficient?

--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2128,10 +2128,18 @@ static int check_map_func_compatibility(struct 
bpf_verifier_env *env,
  		if (env->subprog_cnt > 1) {
  			verbose(env, "tail_calls are not allowed in programs with 
bpf-to-bpf calls\n");
  			return -EINVAL;
  		}
  		break;
+	case BPF_FUNC_map_lookup_elem:
+		if (map->map_type == BPF_MAP_TYPE_PROG_ARRAY ||
+		    map->map_type == BPF_MAP_TYPE_STACK_TRACE ||
+		    map->map_type == BPF_MAP_TYPE_XSKMAP ||
+		    map->map_type == BPF_MAP_TYPE_SOCKMAP ||
+		    map->map_type == BPF_MAP_TYPE_SOCKHASH)
+			goto error;
+		break;
  	case BPF_FUNC_perf_event_read:
  	case BPF_FUNC_perf_event_output:
  	case BPF_FUNC_perf_event_read_value:
  		if (map->map_type != BPF_MAP_TYPE_PERF_EVENT_ARRAY)
  			goto error;

-Prashant

Re: [RFC bpf-next 3/4] tools/bpf: bpftool, split the function do_dump()

[Prashant Bhole]

On 9/20/2018 12:26 AM, Jakub Kicinski wrote:
> On Wed, 19 Sep 2018 16:51:42 +0900, Prashant Bhole wrote:
>> +static int dump_map_elem(int fd, void *key, void *value,
>> +			 struct bpf_map_info *map_info, struct btf *btf,
>> +			 json_writer_t *btf_wtr)
>> +{
>> +	int num_elems = 0;
>> +
>> +	if (!bpf_map_lookup_elem(fd, key, value)) {
>> +		if (json_output) {
>> +			print_entry_json(map_info, key, value, btf);
>> +		} else {
>> +			if (btf) {
>> +				struct btf_dumper d = {
>> +					.btf = btf,
>> +					.jw = btf_wtr,
>> +					.is_plain_text = true,
>> +				};
>> +
>> +				do_dump_btf(&d, map_info, key, value);
>> +			} else {
>> +				print_entry_plain(map_info, key, value);
>> +			}
>> +			num_elems++;
>> +		}
>> +		goto out;
>> +	}
>> +
>> +	/* lookup error handling */
>> +	if (map_is_map_of_maps(map_info->type) ||
>> +	    map_is_map_of_progs(map_info->type))
>> +		goto out;
>> +
> 
> nit: why not just return?  the goto seems to only do a return anyway,
>       is this suggested by some coding style?  Is it to help the
>       compiler?  I see people do this from time to time..

Thanks for reviewing. I agree, goto and the label isn't needed. I will 
fix it.

-Prashant
> 
> [...]
> 
>> +out:
>> +	return num_elems;
> 
> 

Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed

[Prashant Bhole]

On 9/20/2018 12:29 AM, Jakub Kicinski wrote:
> On Wed, 19 Sep 2018 16:51:43 +0900, Prashant Bhole wrote:
>> Let's add a check for EOPNOTSUPP error when map lookup is failed.
>> Also in case map doesn't support lookup, the output of map dump is
>> changed from "can't lookup element" to "lookup not supported for
>> this map".
>>
>> Patch adds function print_entry_error() function to print the error
>> value.
>>
>> Following example dumps a map which does not support lookup.
>>
>> Output before:
>> root# bpftool map -jp dump id 40
>> [
>>      "key": ["0x0a","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "can\'t lookup element"
>>      },
>>      "key": ["0x0b","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "can\'t lookup element"
>>      }
>> ]
>>
>> root# bpftool map dump id 40
>> can't lookup element with key:
>> 0a 00 00 00
>> can't lookup element with key:
>> 0b 00 00 00
>> Found 0 elements
>>
>> Output after changes:
>> root# bpftool map dump -jp  id 45
>> [
>>      "key": ["0x0a","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "lookup not supported for this map"
>>      },
>>      "key": ["0x0b","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "lookup not supported for this map"
>>      }
>> ]
>>
>> root# bpftool map dump id 45
>> key:
>> 0a 00 00 00
>> value:
>> lookup not supported for this map
>> key:
>> 0b 00 00 00
>> value:
>> lookup not supported for this map
>> Found 0 elements
> 
> Nice improvement, thanks for the changes!  I wonder what your thoughts
> would be on just printing some form of "lookup not supported for this
> map" only once?  It seems slightly like repeated information - if
> lookup is not supported for one key it likely won't be for other keys
> too, so we could shorten the output.  Would that make sense?

I too thought that the message is repeated information. One idea was as 
you said, stop iterating once we get EOPNOTSUPP. But only reason for 
keeping this way is that user will at least see dump of keys along with 
it. Also it will be consistent with Json output. What is your opinion on it?

I will fix other things that you pointed out. Thanks!

-Prashant

> 
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> ---
>>   tools/bpf/bpftool/main.h |  5 +++++
>>   tools/bpf/bpftool/map.c  | 35 ++++++++++++++++++++++++++++++-----
>>   2 files changed, 35 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>> index 40492cdc4e53..1a8c683f949b 100644
>> --- a/tools/bpf/bpftool/main.h
>> +++ b/tools/bpf/bpftool/main.h
>> @@ -46,6 +46,11 @@
>>   
>>   #include "json_writer.h"
>>   
>> +#define ERR_CANNOT_LOOKUP \
>> +	"can't lookup element"
>> +#define ERR_LOOKUP_NOT_SUPPORTED \
>> +	"lookup not supported for this map"
> 
> Do we need these?  Are we going to reused them in more parts of the
> code?
> 
>>   #define ptr_to_u64(ptr)	((__u64)(unsigned long)(ptr))
>>   
>>   #define NEXT_ARG()	({ argc--; argv++; if (argc < 0) usage(); })
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index 284e12a289c0..2faccd2098c9 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -333,6 +333,25 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>>   	jsonw_end_object(json_wtr);
>>   }
>>   
>> +static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>> +			      char *value)
>> +{
>> +	bool single_line, break_names;
>> +	int value_size = strlen(value);
> 
> nit: order variables declaration lines to shortest, please.
> 
>> +
>> +	break_names = info->key_size > 16 || value_size > 16;
>> +	single_line = info->key_size + value_size <= 24 && !break_names;
>> +
>> +	printf("key:%c", break_names ? '\n' : ' ');
>> +	fprint_hex(stdout, key, info->key_size, " ");
>> +
>> +	printf(single_line ? "  " : "\n");
>> +
>> +	printf("value:%c%s", break_names ? '\n' : ' ', value);
>> +
>> +	printf("\n");
>> +}
>> +
>>   static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>>   			      unsigned char *value)
>>   {
>> @@ -660,6 +679,8 @@ static int dump_map_elem(int fd, void *key, void *value,
>>   			 json_writer_t *btf_wtr)
>>   {
>>   	int num_elems = 0;
>> +	int lookup_errno;
>> +	char *errstr;
> 
> nit: const char?
> 
>>   
>>   	if (!bpf_map_lookup_elem(fd, key, value)) {
>>   		if (json_output) {
> 
> 
> 

Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed

[Prashant Bhole]

On 9/20/2018 12:29 AM, Jakub Kicinski wrote:
> On Wed, 19 Sep 2018 16:51:43 +0900, Prashant Bhole wrote:
>> Let's add a check for EOPNOTSUPP error when map lookup is failed.
>> Also in case map doesn't support lookup, the output of map dump is
>> changed from "can't lookup element" to "lookup not supported for
>> this map".
>>
>> Patch adds function print_entry_error() function to print the error
>> value.
>>
>> Following example dumps a map which does not support lookup.
>>
>> Output before:
>> root# bpftool map -jp dump id 40
>> [
>>      "key": ["0x0a","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "can\'t lookup element"
>>      },
>>      "key": ["0x0b","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "can\'t lookup element"
>>      }
>> ]
>>
>> root# bpftool map dump id 40
>> can't lookup element with key:
>> 0a 00 00 00
>> can't lookup element with key:
>> 0b 00 00 00
>> Found 0 elements
>>
>> Output after changes:
>> root# bpftool map dump -jp  id 45
>> [
>>      "key": ["0x0a","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "lookup not supported for this map"
>>      },
>>      "key": ["0x0b","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "lookup not supported for this map"
>>      }
>> ]
>>
>> root# bpftool map dump id 45
>> key:
>> 0a 00 00 00
>> value:
>> lookup not supported for this map
>> key:
>> 0b 00 00 00
>> value:
>> lookup not supported for this map
>> Found 0 elements
> 
> Nice improvement, thanks for the changes!  I wonder what your thoughts
> would be on just printing some form of "lookup not supported for this
> map" only once?  It seems slightly like repeated information - if
> lookup is not supported for one key it likely won't be for other keys
> too, so we could shorten the output.  Would that make sense?
> 
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> ---
>>   tools/bpf/bpftool/main.h |  5 +++++
>>   tools/bpf/bpftool/map.c  | 35 ++++++++++++++++++++++++++++++-----
>>   2 files changed, 35 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>> index 40492cdc4e53..1a8c683f949b 100644
>> --- a/tools/bpf/bpftool/main.h
>> +++ b/tools/bpf/bpftool/main.h
>> @@ -46,6 +46,11 @@
>>   
>>   #include "json_writer.h"
>>   
>> +#define ERR_CANNOT_LOOKUP \
>> +	"can't lookup element"
>> +#define ERR_LOOKUP_NOT_SUPPORTED \
>> +	"lookup not supported for this map"
> 
> Do we need these?  Are we going to reused them in more parts of the
> code?

These are used only once. These can be used in do_lookup(). Currently 
do_lookup() prints strerror(errno) when lookup is failed. Shall I change 
that do_lookup() output?

-Prashant

Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed

[Jakub Kicinski]

On Thu, 20 Sep 2018 14:04:19 +0900, Prashant Bhole wrote:
> On 9/20/2018 12:29 AM, Jakub Kicinski wrote:
> > On Wed, 19 Sep 2018 16:51:43 +0900, Prashant Bhole wrote:  
> >> Let's add a check for EOPNOTSUPP error when map lookup is failed.
> >> Also in case map doesn't support lookup, the output of map dump is
> >> changed from "can't lookup element" to "lookup not supported for
> >> this map".
> >>
> >> Patch adds function print_entry_error() function to print the error
> >> value.
> >>
> >> Following example dumps a map which does not support lookup.
> >>
> >> Output before:
> >> root# bpftool map -jp dump id 40
> >> [
> >>      "key": ["0x0a","0x00","0x00","0x00"
> >>      ],
> >>      "value": {
> >>          "error": "can\'t lookup element"
> >>      },
> >>      "key": ["0x0b","0x00","0x00","0x00"
> >>      ],
> >>      "value": {
> >>          "error": "can\'t lookup element"
> >>      }
> >> ]
> >>
> >> root# bpftool map dump id 40
> >> can't lookup element with key:
> >> 0a 00 00 00
> >> can't lookup element with key:
> >> 0b 00 00 00
> >> Found 0 elements
> >>
> >> Output after changes:
> >> root# bpftool map dump -jp  id 45
> >> [
> >>      "key": ["0x0a","0x00","0x00","0x00"
> >>      ],
> >>      "value": {
> >>          "error": "lookup not supported for this map"
> >>      },
> >>      "key": ["0x0b","0x00","0x00","0x00"
> >>      ],
> >>      "value": {
> >>          "error": "lookup not supported for this map"
> >>      }
> >> ]
> >>
> >> root# bpftool map dump id 45
> >> key:
> >> 0a 00 00 00
> >> value:
> >> lookup not supported for this map
> >> key:
> >> 0b 00 00 00
> >> value:
> >> lookup not supported for this map
> >> Found 0 elements  
> > 
> > Nice improvement, thanks for the changes!  I wonder what your thoughts
> > would be on just printing some form of "lookup not supported for this
> > map" only once?  It seems slightly like repeated information - if
> > lookup is not supported for one key it likely won't be for other keys
> > too, so we could shorten the output.  Would that make sense?
> >   
> >> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> >> ---
> >>   tools/bpf/bpftool/main.h |  5 +++++
> >>   tools/bpf/bpftool/map.c  | 35 ++++++++++++++++++++++++++++++-----
> >>   2 files changed, 35 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> >> index 40492cdc4e53..1a8c683f949b 100644
> >> --- a/tools/bpf/bpftool/main.h
> >> +++ b/tools/bpf/bpftool/main.h
> >> @@ -46,6 +46,11 @@
> >>   
> >>   #include "json_writer.h"
> >>   
> >> +#define ERR_CANNOT_LOOKUP \
> >> +	"can't lookup element"
> >> +#define ERR_LOOKUP_NOT_SUPPORTED \
> >> +	"lookup not supported for this map"  
> > 
> > Do we need these?  Are we going to reused them in more parts of the
> > code?  
> 
> These are used only once. These can be used in do_lookup(). Currently 
> do_lookup() prints strerror(errno) when lookup is failed. Shall I change 
> that do_lookup() output?

I actually prefer to stick to strerror(), the standard errors more
clearly correlate with what happened in my mind (i.e. "Operation not
supported" == kernel sent EOPNOTSUPP).   strerror() may also print in
local language if translation/localization matters.

We could even use strerr() in dump_map_elem() but up to you.  The one
in do_lookup() I'd prefer to leave be ;)

Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed

[Prashant Bhole]

On 9/21/2018 12:59 AM, Jakub Kicinski wrote:
> On Thu, 20 Sep 2018 14:04:19 +0900, Prashant Bhole wrote:
>> On 9/20/2018 12:29 AM, Jakub Kicinski wrote:
>>> On Wed, 19 Sep 2018 16:51:43 +0900, Prashant Bhole wrote:
>>>> Let's add a check for EOPNOTSUPP error when map lookup is failed.
>>>> Also in case map doesn't support lookup, the output of map dump is
>>>> changed from "can't lookup element" to "lookup not supported for
>>>> this map".
>>>>
>>>> Patch adds function print_entry_error() function to print the error
>>>> value.
>>>>
>>>> Following example dumps a map which does not support lookup.
>>>>
>>>> Output before:
>>>> root# bpftool map -jp dump id 40
>>>> [
>>>>       "key": ["0x0a","0x00","0x00","0x00"
>>>>       ],
>>>>       "value": {
>>>>           "error": "can\'t lookup element"
>>>>       },
>>>>       "key": ["0x0b","0x00","0x00","0x00"
>>>>       ],
>>>>       "value": {
>>>>           "error": "can\'t lookup element"
>>>>       }
>>>> ]
>>>>
>>>> root# bpftool map dump id 40
>>>> can't lookup element with key:
>>>> 0a 00 00 00
>>>> can't lookup element with key:
>>>> 0b 00 00 00
>>>> Found 0 elements
>>>>
>>>> Output after changes:
>>>> root# bpftool map dump -jp  id 45
>>>> [
>>>>       "key": ["0x0a","0x00","0x00","0x00"
>>>>       ],
>>>>       "value": {
>>>>           "error": "lookup not supported for this map"
>>>>       },
>>>>       "key": ["0x0b","0x00","0x00","0x00"
>>>>       ],
>>>>       "value": {
>>>>           "error": "lookup not supported for this map"
>>>>       }
>>>> ]
>>>>
>>>> root# bpftool map dump id 45
>>>> key:
>>>> 0a 00 00 00
>>>> value:
>>>> lookup not supported for this map
>>>> key:
>>>> 0b 00 00 00
>>>> value:
>>>> lookup not supported for this map
>>>> Found 0 elements
>>>
>>> Nice improvement, thanks for the changes!  I wonder what your thoughts
>>> would be on just printing some form of "lookup not supported for this
>>> map" only once?  It seems slightly like repeated information - if
>>> lookup is not supported for one key it likely won't be for other keys
>>> too, so we could shorten the output.  Would that make sense?
>>>    
>>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>>> ---
>>>>    tools/bpf/bpftool/main.h |  5 +++++
>>>>    tools/bpf/bpftool/map.c  | 35 ++++++++++++++++++++++++++++++-----
>>>>    2 files changed, 35 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>>> index 40492cdc4e53..1a8c683f949b 100644
>>>> --- a/tools/bpf/bpftool/main.h
>>>> +++ b/tools/bpf/bpftool/main.h
>>>> @@ -46,6 +46,11 @@
>>>>    
>>>>    #include "json_writer.h"
>>>>    
>>>> +#define ERR_CANNOT_LOOKUP \
>>>> +	"can't lookup element"
>>>> +#define ERR_LOOKUP_NOT_SUPPORTED \
>>>> +	"lookup not supported for this map"
>>>
>>> Do we need these?  Are we going to reused them in more parts of the
>>> code?
>>
>> These are used only once. These can be used in do_lookup(). Currently
>> do_lookup() prints strerror(errno) when lookup is failed. Shall I change
>> that do_lookup() output?
> 
> I actually prefer to stick to strerror(), the standard errors more
> clearly correlate with what happened in my mind (i.e. "Operation not
> supported" == kernel sent EOPNOTSUPP).   strerror() may also print in
> local language if translation/localization matters.
> 
> We could even use strerr() in dump_map_elem() but up to you.  The one
> in do_lookup() I'd prefer to leave be ;)

Sorry for the late reply.
In v2 I have removed the error strings altogether. As you suggested 
output will be strerror(). Also added verifier tests as Alexei 
suggested. I am sending the RFC-v2 series soon.
Thanks.

-Prashant