Re: [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Mon, 9 Jul 2018 21:23:20 -0700, Andrey Ignatov wrote:
> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 19:49 -0700]:
> > On Mon, 9 Jul 2018 13:22:54 -0700, Andrey Ignatov wrote:  
> > > Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 11:01 -0700]:  
> > > fd of every map is set to -1 in bpf_object__init_maps() that, in turn, is
> > > called from __bpf_object__open():
> > > 
> > > 	for (i = 0; i < nr_maps; i++)
> > > 		obj->maps[i].fd = -1;
> > > 
> > > Later it will either contain valid fd that is >= 0, or that same -1, what
> > > should be enough to identify fd presence.  
> > 
> > I thought it to be cleaner to indicate the fd has been pre-set, in case
> > things get more complicated in the future and fd >= 0 becomes ambiguous.
> > 
> > But no strong preference, should I change?  
> 
> My preference (not strong either) is to avoid a new field whenever it's
> possible. Though if you have a use-case that can't be covered by
> (fd >= 0) keeping the field is fine as well.

Okay, I can change.  I think it may be worthwhile keeping the
information that the map definition was replaced by something that did
not come from the ELF file, but I don't actually have a use for it
right now, so we can just add it back later :)

> > > > +	map->fd = dup(fd);    
> > > 
> > > Unfortunately, new descriptor created by dup(2) will not have O_CLOEXEC set, in
> > > contrast to original fd returned by kernel on map creation.
> > > 
> > > libbpf has other interface shortcomings where it comes up. E.g. struct
> > > bpf_object owns all descriptors it contains (progs, maps) and closes them in
> > > bpf_object__close(). if one wants to open/load ELF, then close it but
> > > keep, say, prog fd to attach it to cgroup some time later, then fd
> > > should be duplicated as well to get a new one not owned by bpf_object.
> > > 
> > > Currently I use this workaround to avoid time when new fd doesn't have
> > > O_CLOEXEC:
> > > 
> > > 	int new_prog_fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
> > > 	if (new_prog_fd < 0 ||
> > > 	    dup3(bpf_program__fd(prog), new_prog_fd, O_CLOEXEC) == -1) {
> > > 		/* .. handle error .. */
> > > 		close(new_prog_fd);
> > > 	}
> > > 	/* .. use new_prog_fd with O_CLOEXEC set */
> > > 
> > > Not sure how to simplify it. dup2() has same problem with regard to
> > > O_CLOEXEC.
> > > 
> > > Use-case: standalone server application that uses libbpf and does
> > > fork()/execve() a lot.  
> > 
> > Good point!  I have no better ideas.  Although being slightly paranoid
> > I would perhaps use "/" instead of "/dev/null"?  Shouldn't matter?  
> 
> No strong preferences, important thing is to create fd with O_CLOEXEC
> set somehow.
> 
> Is it safer to use "/" than "/dev/null"? (trying to understand if I
> should change my code as well)

IDK :)  Could there be a crazy scenario when someone runs chroot or a
very broken system without /dev/null?  / should always be there?

Thanks for all the other reviews, I will update the code accordingly!