| Message ID | 20240316091026.11164-1-mg@max.gautier.name (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Stephen Hemminger |
| Headers | show |
| Series | [iproute2-next] arpd: create /var/lib/arpd on first use | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Sat, 16 Mar 2024 10:06:44 +0100 Max Gautier <mg@max.gautier.name> wrote: > The motivation is to build distributions packages without /var to go > towards stateless systems, see link below (TL;DR: provisionning anything > outside of /usr on boot). > > We only try do create the database directory when it's in the default > location, and assume its parent (/var/lib in the usual case) exists. > > Links: https://0pointer.net/blog/projects/stateless.html > --- > Instead of modifying the default location, I opted to create it at > runtime, but only for the default location and assuming that /var/lib > exists. My thinking is that not changing defaults is somewhat better, > plus using /var/tmp directly might cause security concerns (I don't know > that it does, but at least someone could create a db file which the root > user would then open by default. Not sure what that could cause, so I'd > rather avoid it). > > Makefile | 2 +- > misc/arpd.c | 12 +++++++++++- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 8024d45e..2b2c3dec 100644 > --- a/Makefile > +++ b/Makefile > @@ -42,6 +42,7 @@ DEFINES+=-DCONF_USR_DIR=\"$(CONF_USR_DIR)\" \ > -DCONF_ETC_DIR=\"$(CONF_ETC_DIR)\" \ > -DNETNS_RUN_DIR=\"$(NETNS_RUN_DIR)\" \ > -DNETNS_ETC_DIR=\"$(NETNS_ETC_DIR)\" \ > + -DARPDDIR=\"$(ARPDDIR)\" \ > -DCONF_COLOR=$(CONF_COLOR) > > #options for AX.25 > @@ -104,7 +105,6 @@ config.mk: > install: all > install -m 0755 -d $(DESTDIR)$(SBINDIR) > install -m 0755 -d $(DESTDIR)$(CONF_USR_DIR) > - install -m 0755 -d $(DESTDIR)$(ARPDDIR) > install -m 0755 -d $(DESTDIR)$(HDRDIR) > @for i in $(SUBDIRS); do $(MAKE) -C $$i install; done > install -m 0644 $(shell find etc/iproute2 -maxdepth 1 -type f) $(DESTDIR)$(CONF_USR_DIR) > diff --git a/misc/arpd.c b/misc/arpd.c > index 1ef837c6..a133226c 100644 > --- a/misc/arpd.c > +++ b/misc/arpd.c > @@ -19,6 +19,7 @@ > #include <fcntl.h> > #include <sys/uio.h> > #include <sys/socket.h> > +#include <sys/stat.h> > #include <sys/time.h> > #include <time.h> > #include <signal.h> > @@ -35,7 +36,8 @@ > #include "rt_names.h" > > DB *dbase; > -char *dbname = "/var/lib/arpd/arpd.db"; > +char const * const default_dbname = ARPDDIR "/arpd.db"; Make this an array. const char *default_dbname[] = ARPDDIR "/arpd.db"; > +char const *dbname = default_dbname; > > int ifnum; > int *ifvec; > @@ -668,6 +670,14 @@ int main(int argc, char **argv) > } > } > > + if (default_dbname == dbname > + && mkdir(ARPDDIR, 0755) != 0 > + && errno != EEXIST > + ) { > + perror("create_db_dir"); > + exit(-1); > + } > + > dbase = dbopen(dbname, O_CREAT|O_RDWR, 0644, DB_HASH, NULL); > if (dbase == NULL) { > perror("db_open"); Missing signed-off-by
Stephen Hemminger <stephen@networkplumber.org> wrote: >On Sat, 16 Mar 2024 10:06:44 +0100 >Max Gautier <mg@max.gautier.name> wrote: > >> The motivation is to build distributions packages without /var to go >> towards stateless systems, see link below (TL;DR: provisionning anything >> outside of /usr on boot). >> >> We only try do create the database directory when it's in the default >> location, and assume its parent (/var/lib in the usual case) exists. >> >> Links: https://0pointer.net/blog/projects/stateless.html >> --- >> Instead of modifying the default location, I opted to create it at >> runtime, but only for the default location and assuming that /var/lib >> exists. My thinking is that not changing defaults is somewhat better, >> plus using /var/tmp directly might cause security concerns (I don't know >> that it does, but at least someone could create a db file which the root >> user would then open by default. Not sure what that could cause, so I'd >> rather avoid it). >> >> Makefile | 2 +- >> misc/arpd.c | 12 +++++++++++- >> 2 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index 8024d45e..2b2c3dec 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -42,6 +42,7 @@ DEFINES+=-DCONF_USR_DIR=\"$(CONF_USR_DIR)\" \ >> -DCONF_ETC_DIR=\"$(CONF_ETC_DIR)\" \ >> -DNETNS_RUN_DIR=\"$(NETNS_RUN_DIR)\" \ >> -DNETNS_ETC_DIR=\"$(NETNS_ETC_DIR)\" \ >> + -DARPDDIR=\"$(ARPDDIR)\" \ >> -DCONF_COLOR=$(CONF_COLOR) >> >> #options for AX.25 >> @@ -104,7 +105,6 @@ config.mk: >> install: all >> install -m 0755 -d $(DESTDIR)$(SBINDIR) >> install -m 0755 -d $(DESTDIR)$(CONF_USR_DIR) >> - install -m 0755 -d $(DESTDIR)$(ARPDDIR) >> install -m 0755 -d $(DESTDIR)$(HDRDIR) >> @for i in $(SUBDIRS); do $(MAKE) -C $$i install; done >> install -m 0644 $(shell find etc/iproute2 -maxdepth 1 -type f) $(DESTDIR)$(CONF_USR_DIR) >> diff --git a/misc/arpd.c b/misc/arpd.c >> index 1ef837c6..a133226c 100644 >> --- a/misc/arpd.c >> +++ b/misc/arpd.c >> @@ -19,6 +19,7 @@ >> #include <fcntl.h> >> #include <sys/uio.h> >> #include <sys/socket.h> >> +#include <sys/stat.h> >> #include <sys/time.h> >> #include <time.h> >> #include <signal.h> >> @@ -35,7 +36,8 @@ >> #include "rt_names.h" >> >> DB *dbase; >> -char *dbname = "/var/lib/arpd/arpd.db"; >> +char const * const default_dbname = ARPDDIR "/arpd.db"; > >Make this an array. >const char *default_dbname[] = ARPDDIR "/arpd.db"; I suspect this should be const char default_dbname[] = ARPDDIR "/arpd.db"; i.e., no "*" before "default_dbname", to match the type of dbname (below). >> +char const *dbname = default_dbname; >> >> int ifnum; >> int *ifvec; >> @@ -668,6 +670,14 @@ int main(int argc, char **argv) >> } >> } >> >> + if (default_dbname == dbname >> + && mkdir(ARPDDIR, 0755) != 0 >> + && errno != EEXIST >> + ) { >> + perror("create_db_dir"); >> + exit(-1); >> + } >> + Should this be a string comparison? I don't think the pointer comparison "default_dbname == dbname" will do what you expect if a user specifies "-b" with the default value of ARPDIR "/arpd.db" as its argument (i.e., the pointers won't match, but the actual text is the same). -J >> dbase = dbopen(dbname, O_CREAT|O_RDWR, 0644, DB_HASH, NULL); >> if (dbase == NULL) { >> perror("db_open"); > >Missing signed-off-by > --- -Jay Vosburgh, jay.vosburgh@canonical.com
Le 16 mars 2024 20:56:37 GMT+01:00, Jay Vosburgh <jay.vosburgh@canonical.com> a écrit : >Stephen Hemminger <stephen@networkplumber.org> wrote: > >>On Sat, 16 Mar 2024 10:06:44 +0100 >>Max Gautier <mg@max.gautier.name> wrote: >> >>> The motivation is to build distributions packages without /var to go >>> towards stateless systems, see link below (TL;DR: provisionning anything >>> outside of /usr on boot). >>> >>> We only try do create the database directory when it's in the default >>> location, and assume its parent (/var/lib in the usual case) exists. >>> >>> Links: https://0pointer.net/blog/projects/stateless.html >>> --- >>> Instead of modifying the default location, I opted to create it at >>> runtime, but only for the default location and assuming that /var/lib >>> exists. My thinking is that not changing defaults is somewhat better, >>> plus using /var/tmp directly might cause security concerns (I don't know >>> that it does, but at least someone could create a db file which the root >>> user would then open by default. Not sure what that could cause, so I'd >>> rather avoid it). >>> >>> Makefile | 2 +- >>> misc/arpd.c | 12 +++++++++++- >>> 2 files changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/Makefile b/Makefile >>> index 8024d45e..2b2c3dec 100644 >>> --- a/Makefile >>> +++ b/Makefile >>> @@ -42,6 +42,7 @@ DEFINES+=-DCONF_USR_DIR=\"$(CONF_USR_DIR)\" \ >>> -DCONF_ETC_DIR=\"$(CONF_ETC_DIR)\" \ >>> -DNETNS_RUN_DIR=\"$(NETNS_RUN_DIR)\" \ >>> -DNETNS_ETC_DIR=\"$(NETNS_ETC_DIR)\" \ >>> + -DARPDDIR=\"$(ARPDDIR)\" \ >>> -DCONF_COLOR=$(CONF_COLOR) >>> >>> #options for AX.25 >>> @@ -104,7 +105,6 @@ config.mk: >>> install: all >>> install -m 0755 -d $(DESTDIR)$(SBINDIR) >>> install -m 0755 -d $(DESTDIR)$(CONF_USR_DIR) >>> - install -m 0755 -d $(DESTDIR)$(ARPDDIR) >>> install -m 0755 -d $(DESTDIR)$(HDRDIR) >>> @for i in $(SUBDIRS); do $(MAKE) -C $$i install; done >>> install -m 0644 $(shell find etc/iproute2 -maxdepth 1 -type f) $(DESTDIR)$(CONF_USR_DIR) >>> diff --git a/misc/arpd.c b/misc/arpd.c >>> index 1ef837c6..a133226c 100644 >>> --- a/misc/arpd.c >>> +++ b/misc/arpd.c >>> @@ -19,6 +19,7 @@ >>> #include <fcntl.h> >>> #include <sys/uio.h> >>> #include <sys/socket.h> >>> +#include <sys/stat.h> >>> #include <sys/time.h> >>> #include <time.h> >>> #include <signal.h> >>> @@ -35,7 +36,8 @@ >>> #include "rt_names.h" >>> >>> DB *dbase; >>> -char *dbname = "/var/lib/arpd/arpd.db"; >>> +char const * const default_dbname = ARPDDIR "/arpd.db"; >> >>Make this an array. >>const char *default_dbname[] = ARPDDIR "/arpd.db"; > > I suspect this should be > >const char default_dbname[] = ARPDDIR "/arpd.db"; > > i.e., no "*" before "default_dbname", to match the type of >dbname (below). Yeah, does not compile otherwise, that was my guess as well. > >>> +char const *dbname = default_dbname; >>> >>> int ifnum; >>> int *ifvec; >>> @@ -668,6 +670,14 @@ int main(int argc, char **argv) >>> } >>> } >>> >>> + if (default_dbname == dbname >>> + && mkdir(ARPDDIR, 0755) != 0 >>> + && errno != EEXIST >>> + ) { >>> + perror("create_db_dir"); >>> + exit(-1); >>> + } >>> + > > Should this be a string comparison? I don't think the pointer >comparison "default_dbname == dbname" will do what you expect if a user >specifies "-b" with the default value of ARPDIR "/arpd.db" as its >argument (i.e., the pointers won't match, but the actual text is the >same). > > -J I did consider that, the reasoning was mostly something like "if you specify a custom location, you're on your own". But a string compare is probably best, it preserve the observable behavior of arpd and is overall less surprising. I'll send a v2 shortly. > >>> dbase = dbopen(dbname, O_CREAT|O_RDWR, 0644, DB_HASH, NULL); >>> if (dbase == NULL) { >>> perror("db_open"); >> >>Missing signed-off-by >> > >--- > -Jay Vosburgh, jay.vosburgh@canonical.com
diff --git a/Makefile b/Makefile index 8024d45e..2b2c3dec 100644 --- a/Makefile +++ b/Makefile @@ -42,6 +42,7 @@ DEFINES+=-DCONF_USR_DIR=\"$(CONF_USR_DIR)\" \ -DCONF_ETC_DIR=\"$(CONF_ETC_DIR)\" \ -DNETNS_RUN_DIR=\"$(NETNS_RUN_DIR)\" \ -DNETNS_ETC_DIR=\"$(NETNS_ETC_DIR)\" \ + -DARPDDIR=\"$(ARPDDIR)\" \ -DCONF_COLOR=$(CONF_COLOR) #options for AX.25 @@ -104,7 +105,6 @@ config.mk: install: all install -m 0755 -d $(DESTDIR)$(SBINDIR) install -m 0755 -d $(DESTDIR)$(CONF_USR_DIR) - install -m 0755 -d $(DESTDIR)$(ARPDDIR) install -m 0755 -d $(DESTDIR)$(HDRDIR) @for i in $(SUBDIRS); do $(MAKE) -C $$i install; done install -m 0644 $(shell find etc/iproute2 -maxdepth 1 -type f) $(DESTDIR)$(CONF_USR_DIR) diff --git a/misc/arpd.c b/misc/arpd.c index 1ef837c6..a133226c 100644 --- a/misc/arpd.c +++ b/misc/arpd.c @@ -19,6 +19,7 @@ #include <fcntl.h> #include <sys/uio.h> #include <sys/socket.h> +#include <sys/stat.h> #include <sys/time.h> #include <time.h> #include <signal.h> @@ -35,7 +36,8 @@ #include "rt_names.h" DB *dbase; -char *dbname = "/var/lib/arpd/arpd.db"; +char const * const default_dbname = ARPDDIR "/arpd.db"; +char const *dbname = default_dbname; int ifnum; int *ifvec; @@ -668,6 +670,14 @@ int main(int argc, char **argv) } } + if (default_dbname == dbname + && mkdir(ARPDDIR, 0755) != 0 + && errno != EEXIST + ) { + perror("create_db_dir"); + exit(-1); + } + dbase = dbopen(dbname, O_CREAT|O_RDWR, 0644, DB_HASH, NULL); if (dbase == NULL) { perror("db_open");