bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] bpftool: Try to read btf as raw data if elf read fails
@ 2019-10-18 10:34 Jiri Olsa
  2019-10-18 16:48 ` Andrii Nakryiko
  2019-10-18 22:39 ` Jakub Kicinski
  0 siblings, 2 replies; 8+ messages in thread
From: Jiri Olsa @ 2019-10-18 10:34 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: netdev, bpf, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau

The bpftool interface stays the same, but now it's possible
to run it over BTF raw data, like:

  $ bpftool btf dump file /sys/kernel/btf/vmlinux
  libbpf: failed to get EHDR from /sys/kernel/btf/vmlinux
  [1] INT '(anon)' size=4 bits_offset=0 nr_bits=32 encoding=(none)
  [2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
  [3] CONST '(anon)' type_id=2

I'm also adding err init to 0 because I was getting uninitialized
warnings from gcc.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/bpf/bpftool/btf.c | 47 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 9a9376d1d3df..100fb7e02329 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -12,6 +12,9 @@
 #include <libbpf.h>
 #include <linux/btf.h>
 #include <linux/hashtable.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
 
 #include "btf.h"
 #include "json_writer.h"
@@ -388,6 +391,35 @@ static int dump_btf_c(const struct btf *btf,
 	return err;
 }
 
+static struct btf *btf__parse_raw(const char *file)
+{
+	struct btf *btf = ERR_PTR(-EINVAL);
+	__u8 *buf = NULL;
+	struct stat st;
+	FILE *f;
+
+	if (stat(file, &st))
+		return btf;
+
+	f = fopen(file, "rb");
+	if (!f)
+		return btf;
+
+	buf = malloc(st.st_size);
+	if (!buf)
+		goto err;
+
+	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f))
+		goto err;
+
+	btf = btf__new(buf, st.st_size);
+
+err:
+	free(buf);
+	fclose(f);
+	return btf;
+}
+
 static int do_dump(int argc, char **argv)
 {
 	struct btf *btf = NULL;
@@ -397,7 +429,7 @@ static int do_dump(int argc, char **argv)
 	__u32 btf_id = -1;
 	const char *src;
 	int fd = -1;
-	int err;
+	int err = 0;
 
 	if (!REQ_ARGS(2)) {
 		usage();
@@ -468,10 +500,15 @@ static int do_dump(int argc, char **argv)
 		btf = btf__parse_elf(*argv, NULL);
 		if (IS_ERR(btf)) {
 			err = PTR_ERR(btf);
-			btf = NULL;
-			p_err("failed to load BTF from %s: %s", 
-			      *argv, strerror(err));
-			goto done;
+			if (err == -LIBBPF_ERRNO__FORMAT)
+				btf = btf__parse_raw(*argv);
+			if (IS_ERR(btf)) {
+				btf = NULL;
+				/* Display the original error value. */
+				p_err("failed to load BTF from %s: %s",
+				      *argv, strerror(err));
+				goto done;
+			}
 		}
 		NEXT_ARG();
 	} else {
-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] bpftool: Try to read btf as raw data if elf read fails
  2019-10-18 10:34 [PATCH] bpftool: Try to read btf as raw data if elf read fails Jiri Olsa
@ 2019-10-18 16:48 ` Andrii Nakryiko
  2019-10-18 20:04   ` Yonghong Song
  2019-10-21 13:54   ` Jiri Olsa
  2019-10-18 22:39 ` Jakub Kicinski
  1 sibling, 2 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2019-10-18 16:48 UTC (permalink / raw)
  To: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann
  Cc: netdev, bpf, Yonghong Song, Martin Lau

On 10/18/19 3:34 AM, Jiri Olsa wrote:
> The bpftool interface stays the same, but now it's possible
> to run it over BTF raw data, like:

Oh, great, I had similar patch laying around for a while, never got to 
cleaning it up, though, so thanks for picking this up!

> 
>    $ bpftool btf dump file /sys/kernel/btf/vmlinux
>    libbpf: failed to get EHDR from /sys/kernel/btf/vmlinux

We should implement this so that we don't get an extra log output with 
errors. I've been thinking about checking first few bytes of the file. 
If that matches BTF_MAGIC, then try to parse it as raw BTF, otherwise 
parse as ELF w/ BTF. Does it make sense?

>    [1] INT '(anon)' size=4 bits_offset=0 nr_bits=32 encoding=(none)
>    [2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
>    [3] CONST '(anon)' type_id=2
> 
> I'm also adding err init to 0 because I was getting uninitialized
> warnings from gcc.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   tools/bpf/bpftool/btf.c | 47 ++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 9a9376d1d3df..100fb7e02329 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -12,6 +12,9 @@
>   #include <libbpf.h>
>   #include <linux/btf.h>
>   #include <linux/hashtable.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
>   
>   #include "btf.h"
>   #include "json_writer.h"
> @@ -388,6 +391,35 @@ static int dump_btf_c(const struct btf *btf,
>   	return err;
>   }
>   
> +static struct btf *btf__parse_raw(const char *file)
> +{
> +	struct btf *btf = ERR_PTR(-EINVAL);
> +	__u8 *buf = NULL;
> +	struct stat st;
> +	FILE *f;
> +
> +	if (stat(file, &st))
> +		return btf;
> +
> +	f = fopen(file, "rb");
> +	if (!f)
> +		return btf;
> +
> +	buf = malloc(st.st_size);
> +	if (!buf)
> +		goto err;
> +
> +	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f))
> +		goto err;
> +
> +	btf = btf__new(buf, st.st_size);
> +
> +err:
> +	free(buf);
> +	fclose(f);
> +	return btf;
> +}
> +
>   static int do_dump(int argc, char **argv)
>   {
>   	struct btf *btf = NULL;
> @@ -397,7 +429,7 @@ static int do_dump(int argc, char **argv)
>   	__u32 btf_id = -1;
>   	const char *src;
>   	int fd = -1;
> -	int err;
> +	int err = 0;
>   
>   	if (!REQ_ARGS(2)) {
>   		usage();
> @@ -468,10 +500,15 @@ static int do_dump(int argc, char **argv)
>   		btf = btf__parse_elf(*argv, NULL);
>   		if (IS_ERR(btf)) {
>   			err = PTR_ERR(btf);
> -			btf = NULL;
> -			p_err("failed to load BTF from %s: %s",
> -			      *argv, strerror(err));
> -			goto done;
> +			if (err == -LIBBPF_ERRNO__FORMAT)
> +				btf = btf__parse_raw(*argv);
> +			if (IS_ERR(btf)) {
> +				btf = NULL;
> +				/* Display the original error value. */
> +				p_err("failed to load BTF from %s: %s",
> +				      *argv, strerror(err));
> +				goto done;
> +			}
>   		}
>   		NEXT_ARG();
>   	} else {
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] bpftool: Try to read btf as raw data if elf read fails
  2019-10-18 16:48 ` Andrii Nakryiko
@ 2019-10-18 20:04   ` Yonghong Song
  2019-10-21 13:55     ` Jiri Olsa
  2019-10-21 13:54   ` Jiri Olsa
  1 sibling, 1 reply; 8+ messages in thread
From: Yonghong Song @ 2019-10-18 20:04 UTC (permalink / raw)
  To: Andrii Nakryiko, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann
  Cc: netdev, bpf, Martin Lau



On 10/18/19 9:48 AM, Andrii Nakryiko wrote:
> On 10/18/19 3:34 AM, Jiri Olsa wrote:
>> The bpftool interface stays the same, but now it's possible
>> to run it over BTF raw data, like:
> 
> Oh, great, I had similar patch laying around for a while, never got to
> cleaning it up, though, so thanks for picking this up!
> 
>>
>>     $ bpftool btf dump file /sys/kernel/btf/vmlinux
>>     libbpf: failed to get EHDR from /sys/kernel/btf/vmlinux
> 
> We should implement this so that we don't get an extra log output with
> errors. I've been thinking about checking first few bytes of the file.
> If that matches BTF_MAGIC, then try to parse it as raw BTF, otherwise
> parse as ELF w/ BTF. Does it make sense?

Agreed, this makes sense. We should not emit errors in such cases.
One minor comment below.

> 
>>     [1] INT '(anon)' size=4 bits_offset=0 nr_bits=32 encoding=(none)
>>     [2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
>>     [3] CONST '(anon)' type_id=2
>>
>> I'm also adding err init to 0 because I was getting uninitialized
>> warnings from gcc.
>>
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> ---
>>    tools/bpf/bpftool/btf.c | 47 ++++++++++++++++++++++++++++++++++++-----
>>    1 file changed, 42 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>> index 9a9376d1d3df..100fb7e02329 100644
>> --- a/tools/bpf/bpftool/btf.c
>> +++ b/tools/bpf/bpftool/btf.c
>> @@ -12,6 +12,9 @@
>>    #include <libbpf.h>
>>    #include <linux/btf.h>
>>    #include <linux/hashtable.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <unistd.h>
>>    
>>    #include "btf.h"
>>    #include "json_writer.h"
>> @@ -388,6 +391,35 @@ static int dump_btf_c(const struct btf *btf,
>>    	return err;
>>    }
>>    
>> +static struct btf *btf__parse_raw(const char *file)
>> +{
>> +	struct btf *btf = ERR_PTR(-EINVAL);
>> +	__u8 *buf = NULL;
>> +	struct stat st;
>> +	FILE *f;
>> +
>> +	if (stat(file, &st))
>> +		return btf;
>> +
>> +	f = fopen(file, "rb");
>> +	if (!f)
>> +		return btf;
>> +
>> +	buf = malloc(st.st_size);
>> +	if (!buf)
>> +		goto err;
>> +
>> +	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f))
>> +		goto err;
>> +
>> +	btf = btf__new(buf, st.st_size);
>> +
>> +err:

Non error case can also reach here. Let us change
label to a different name, e.g., "done"?

>> +	free(buf);
>> +	fclose(f);
>> +	return btf;
>> +}
>> +
>>    static int do_dump(int argc, char **argv)
>>    {
>>    	struct btf *btf = NULL;
>> @@ -397,7 +429,7 @@ static int do_dump(int argc, char **argv)
>>    	__u32 btf_id = -1;
>>    	const char *src;
>>    	int fd = -1;
>> -	int err;
>> +	int err = 0;
>>    
>>    	if (!REQ_ARGS(2)) {
>>    		usage();
>> @@ -468,10 +500,15 @@ static int do_dump(int argc, char **argv)
>>    		btf = btf__parse_elf(*argv, NULL);
>>    		if (IS_ERR(btf)) {
>>    			err = PTR_ERR(btf);
>> -			btf = NULL;
>> -			p_err("failed to load BTF from %s: %s",
>> -			      *argv, strerror(err));
>> -			goto done;
>> +			if (err == -LIBBPF_ERRNO__FORMAT)
>> +				btf = btf__parse_raw(*argv);
>> +			if (IS_ERR(btf)) {
>> +				btf = NULL;
>> +				/* Display the original error value. */
>> +				p_err("failed to load BTF from %s: %s",
>> +				      *argv, strerror(err));
>> +				goto done;
>> +			}
>>    		}
>>    		NEXT_ARG();
>>    	} else {
>>
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] bpftool: Try to read btf as raw data if elf read fails
  2019-10-18 10:34 [PATCH] bpftool: Try to read btf as raw data if elf read fails Jiri Olsa
  2019-10-18 16:48 ` Andrii Nakryiko
@ 2019-10-18 22:39 ` Jakub Kicinski
  2019-10-21 14:02   ` Jiri Olsa
  1 sibling, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2019-10-18 22:39 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau

On Fri, 18 Oct 2019 12:34:04 +0200, Jiri Olsa wrote:
> The bpftool interface stays the same, but now it's possible
> to run it over BTF raw data, like:
> 
>   $ bpftool btf dump file /sys/kernel/btf/vmlinux
>   libbpf: failed to get EHDR from /sys/kernel/btf/vmlinux
>   [1] INT '(anon)' size=4 bits_offset=0 nr_bits=32 encoding=(none)
>   [2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
>   [3] CONST '(anon)' type_id=2
> 
> I'm also adding err init to 0 because I was getting uninitialized
> warnings from gcc.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/bpf/bpftool/btf.c | 47 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 9a9376d1d3df..100fb7e02329 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -12,6 +12,9 @@
>  #include <libbpf.h>
>  #include <linux/btf.h>
>  #include <linux/hashtable.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
>  
>  #include "btf.h"
>  #include "json_writer.h"
> @@ -388,6 +391,35 @@ static int dump_btf_c(const struct btf *btf,
>  	return err;
>  }
>  
> +static struct btf *btf__parse_raw(const char *file)
> +{
> +	struct btf *btf = ERR_PTR(-EINVAL);
> +	__u8 *buf = NULL;

Please drop the inits

> +	struct stat st;
> +	FILE *f;
> +
> +	if (stat(file, &st))
> +		return btf;

And return constants here

> +	f = fopen(file, "rb");
> +	if (!f)
> +		return btf;

and here

> +	buf = malloc(st.st_size);
> +	if (!buf)
> +		goto err;

and jump to the right place here.

> +	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f))
> +		goto err;
> +
> +	btf = btf__new(buf, st.st_size);
> +
> +err:

The prefix for error labels which is shared with non-error path is exit_

> +	free(buf);
> +	fclose(f);
> +	return btf;
> +}
> +
>  static int do_dump(int argc, char **argv)
>  {
>  	struct btf *btf = NULL;
> @@ -397,7 +429,7 @@ static int do_dump(int argc, char **argv)
>  	__u32 btf_id = -1;
>  	const char *src;
>  	int fd = -1;
> -	int err;
> +	int err = 0;

This change looks unnecessary.

>  	if (!REQ_ARGS(2)) {
>  		usage();
> @@ -468,10 +500,15 @@ static int do_dump(int argc, char **argv)
>  		btf = btf__parse_elf(*argv, NULL);
>  		if (IS_ERR(btf)) {
>  			err = PTR_ERR(btf);
> -			btf = NULL;
> -			p_err("failed to load BTF from %s: %s", 
> -			      *argv, strerror(err));
> -			goto done;
> +			if (err == -LIBBPF_ERRNO__FORMAT)
> +				btf = btf__parse_raw(*argv);
> +			if (IS_ERR(btf)) {
> +				btf = NULL;
> +				/* Display the original error value. */
> +				p_err("failed to load BTF from %s: %s",
> +				      *argv, strerror(err));
> +				goto done;
> +			}
>  		}
>  		NEXT_ARG();
>  	} else {


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] bpftool: Try to read btf as raw data if elf read fails
  2019-10-18 16:48 ` Andrii Nakryiko
  2019-10-18 20:04   ` Yonghong Song
@ 2019-10-21 13:54   ` Jiri Olsa
  1 sibling, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2019-10-21 13:54 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	Yonghong Song, Martin Lau

On Fri, Oct 18, 2019 at 04:48:25PM +0000, Andrii Nakryiko wrote:
> On 10/18/19 3:34 AM, Jiri Olsa wrote:
> > The bpftool interface stays the same, but now it's possible
> > to run it over BTF raw data, like:
> 
> Oh, great, I had similar patch laying around for a while, never got to 
> cleaning it up, though, so thanks for picking this up!
> 
> > 
> >    $ bpftool btf dump file /sys/kernel/btf/vmlinux
> >    libbpf: failed to get EHDR from /sys/kernel/btf/vmlinux
> 
> We should implement this so that we don't get an extra log output with 
> errors. I've been thinking about checking first few bytes of the file. 
> If that matches BTF_MAGIC, then try to parse it as raw BTF, otherwise 
> parse as ELF w/ BTF. Does it make sense?

ok, sounds good

jirka


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] bpftool: Try to read btf as raw data if elf read fails
  2019-10-18 20:04   ` Yonghong Song
@ 2019-10-21 13:55     ` Jiri Olsa
  0 siblings, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2019-10-21 13:55 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann,
	netdev, bpf, Martin Lau

On Fri, Oct 18, 2019 at 08:04:44PM +0000, Yonghong Song wrote:

SNIP

> >> +	FILE *f;
> >> +
> >> +	if (stat(file, &st))
> >> +		return btf;
> >> +
> >> +	f = fopen(file, "rb");
> >> +	if (!f)
> >> +		return btf;
> >> +
> >> +	buf = malloc(st.st_size);
> >> +	if (!buf)
> >> +		goto err;
> >> +
> >> +	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f))
> >> +		goto err;
> >> +
> >> +	btf = btf__new(buf, st.st_size);
> >> +
> >> +err:
> 
> Non error case can also reach here. Let us change
> label to a different name, e.g., "done"?

ok, will change

thanks,
jirka


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] bpftool: Try to read btf as raw data if elf read fails
  2019-10-18 22:39 ` Jakub Kicinski
@ 2019-10-21 14:02   ` Jiri Olsa
  2019-10-21 23:16     ` Jakub Kicinski
  0 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2019-10-21 14:02 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau

On Fri, Oct 18, 2019 at 03:39:05PM -0700, Jakub Kicinski wrote:
> On Fri, 18 Oct 2019 12:34:04 +0200, Jiri Olsa wrote:
> > The bpftool interface stays the same, but now it's possible
> > to run it over BTF raw data, like:
> > 
> >   $ bpftool btf dump file /sys/kernel/btf/vmlinux
> >   libbpf: failed to get EHDR from /sys/kernel/btf/vmlinux
> >   [1] INT '(anon)' size=4 bits_offset=0 nr_bits=32 encoding=(none)
> >   [2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
> >   [3] CONST '(anon)' type_id=2
> > 
> > I'm also adding err init to 0 because I was getting uninitialized
> > warnings from gcc.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/bpf/bpftool/btf.c | 47 ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 42 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> > index 9a9376d1d3df..100fb7e02329 100644
> > --- a/tools/bpf/bpftool/btf.c
> > +++ b/tools/bpf/bpftool/btf.c
> > @@ -12,6 +12,9 @@
> >  #include <libbpf.h>
> >  #include <linux/btf.h>
> >  #include <linux/hashtable.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <unistd.h>
> >  
> >  #include "btf.h"
> >  #include "json_writer.h"
> > @@ -388,6 +391,35 @@ static int dump_btf_c(const struct btf *btf,
> >  	return err;
> >  }
> >  
> > +static struct btf *btf__parse_raw(const char *file)
> > +{
> > +	struct btf *btf = ERR_PTR(-EINVAL);
> > +	__u8 *buf = NULL;
> 
> Please drop the inits
> 
> > +	struct stat st;
> > +	FILE *f;
> > +
> > +	if (stat(file, &st))
> > +		return btf;
> 
> And return constants here
> 
> > +	f = fopen(file, "rb");
> > +	if (!f)
> > +		return btf;
> 
> and here
> 
> > +	buf = malloc(st.st_size);
> > +	if (!buf)
> > +		goto err;
> 
> and jump to the right place here.
> 
> > +	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f))
> > +		goto err;
> > +
> > +	btf = btf__new(buf, st.st_size);
> > +
> > +err:
> 
> The prefix for error labels which is shared with non-error path is exit_
> 
> > +	free(buf);
> > +	fclose(f);
> > +	return btf;
> > +}
> > +

ok for all above

> >  static int do_dump(int argc, char **argv)
> >  {
> >  	struct btf *btf = NULL;
> > @@ -397,7 +429,7 @@ static int do_dump(int argc, char **argv)
> >  	__u32 btf_id = -1;
> >  	const char *src;
> >  	int fd = -1;
> > -	int err;
> > +	int err = 0;
> 
> This change looks unnecessary.

I'm getting confusing warnings from gcc about this,
but there is a code path where do_dump would return
untouched err:

  do_dump
     int err;

     } else if (is_prefix(src, "file")) {
       btf = btf__parse_elf(*argv, NULL);   // succeeds

     }

     while (argc) {
       if (is_prefix(*argv, "format")) {
       else {                                // in here
          goto done;
       }

     done:
       return err;

thanks,
jirka


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] bpftool: Try to read btf as raw data if elf read fails
  2019-10-21 14:02   ` Jiri Olsa
@ 2019-10-21 23:16     ` Jakub Kicinski
  0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2019-10-21 23:16 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau

On Mon, 21 Oct 2019 16:02:27 +0200, Jiri Olsa wrote:
> > >  static int do_dump(int argc, char **argv)
> > >  {
> > >  	struct btf *btf = NULL;
> > > @@ -397,7 +429,7 @@ static int do_dump(int argc, char **argv)
> > >  	__u32 btf_id = -1;
> > >  	const char *src;
> > >  	int fd = -1;
> > > -	int err;
> > > +	int err = 0;  
> > 
> > This change looks unnecessary.  
> 
> I'm getting confusing warnings from gcc about this,
> but there is a code path where do_dump would return
> untouched err:
> 
>   do_dump
>      int err;
> 
>      } else if (is_prefix(src, "file")) {
>        btf = btf__parse_elf(*argv, NULL);   // succeeds
> 
>      }
> 
>      while (argc) {
>        if (is_prefix(*argv, "format")) {
>        else {                                // in here
>           goto done;
>        }
> 
>      done:
>        return err;

ugh, right those look legit, although unrelated to you change.

err should always be set before jumping to 'done'. The error
setting in this function looks super messy :( Sometimes is returns
errno codes, sometimes positive values, sometimes negative, sometimes
just -1. Sometimes it jumps to 'done' for no good reason, ahh :/

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-10-21 23:16 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 10:34 [PATCH] bpftool: Try to read btf as raw data if elf read fails Jiri Olsa
2019-10-18 16:48 ` Andrii Nakryiko
2019-10-18 20:04   ` Yonghong Song
2019-10-21 13:55     ` Jiri Olsa
2019-10-21 13:54   ` Jiri Olsa
2019-10-18 22:39 ` Jakub Kicinski
2019-10-21 14:02   ` Jiri Olsa
2019-10-21 23:16     ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).