| Message ID | 20240317090134.4219-1-mg@max.gautier.name (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next,v2] arpd: create /var/lib/arpd on first use | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Sun, 17 Mar 2024 10:01:24 +0100 Max Gautier <mg@max.gautier.name> wrote: > + if (strcmp(default_dbname, dbname) == 0 > + && mkdir(ARPDDIR, 0755) != 0 > + && errno != EEXIST > + ) { > + perror("create_db_dir"); > + exit(-1); > + } Please put closing paren on same line after EEXIST
Le 17 mars 2024 17:03:32 GMT+01:00, Meiyong Yu <meiyong.yu@126.com> a écrit : > > >> On Mar 17, 2024, at 17:04, 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 >> Signed-off-by: Max Gautier <mg@max.gautier.name> >> --- >> 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..a64888aa 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 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 (strcmp(default_dbname, dbname) == 0 >> + && 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"); >> -- >> 2.44.0 >> > >if (strcmp(default_dbname, dbname) == 0 > >Do you consider when the input dbname is relative path, for example: ../../var/lib/arpd/arpd.db, anther example: //var//lib//arpd//arpd.db I don't. IMO this is a corner case which isn't worth doing path normalization. Symlinks are not considered either.
On 2024-03-17 at 14:31:24, 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 > Signed-off-by: Max Gautier <mg@max.gautier.name> > --- > 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..a64888aa 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 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 (strcmp(default_dbname, dbname) == 0 > + && mkdir(ARPDDIR, 0755) != 0 > + && errno != EEXIST why do you need errno != EEXIST case ? mkdir() will return error in this case as well. > + ) { > + perror("create_db_dir"); > + exit(-1); > + } > + > dbase = dbopen(dbname, O_CREAT|O_RDWR, 0644, DB_HASH, NULL); > if (dbase == NULL) { > perror("db_open"); > -- > 2.44.0 >
On Mon, Mar 18, 2024 at 08:26:13AM +0530, Ratheesh Kannoth wrote: > On 2024-03-17 at 14:31:24, 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 > > Signed-off-by: Max Gautier <mg@max.gautier.name> > > --- > > 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..a64888aa 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 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 (strcmp(default_dbname, dbname) == 0 > > + && mkdir(ARPDDIR, 0755) != 0 > > + && errno != EEXIST > why do you need errno != EEXIST case ? mkdir() will return error in this case as well. EEXIST is not an error in this case: if the default location already exist, all is good. mkdir would still return -1 in this case, so we need to exclude it manually. > > + ) { > > + perror("create_db_dir"); > > + exit(-1); > > + } > > + > > dbase = dbopen(dbname, O_CREAT|O_RDWR, 0644, DB_HASH, NULL); > > if (dbase == NULL) { > > perror("db_open"); > > -- > > 2.44.0 > >
> From: Max Gautier <mg@max.gautier.name> > Sent: Monday, March 18, 2024 2:07 PM > To: Ratheesh Kannoth <rkannoth@marvell.com> > Cc: netdev@vger.kernel.org > Subject: [EXTERNAL] Re: [PATCH iproute2-next v2] arpd: create /var/lib/arpd > on first use > > > + if (strcmp(default_dbname, dbname) == 0 > > > + && mkdir(ARPDDIR, 0755) != 0 > > > + && errno != EEXIST > > why do you need errno != EEXIST case ? mkdir() will return error in this case > as well. > > EEXIST is not an error in this case: if the default location already exist, all is > good. mkdir would still return -1 in this case, so we need to exclude it > manually. ACK. IMO, it would make a more readable code if you consider splitting the "if" loop. > > > > + ) { > > > + perror("create_db_dir"); > > > + exit(-1); > > > + } > > > + > > > dbase = dbopen(dbname, O_CREAT|O_RDWR, 0644, DB_HASH, > NULL); > > > if (dbase == NULL) { > > > perror("db_open"); > > > -- > > > 2.44.0 > > > > > -- > Max Gautier
On Mon, Mar 18, 2024 at 08:51:40AM +0000, Ratheesh Kannoth wrote: > > From: Max Gautier <mg@max.gautier.name> > > Sent: Monday, March 18, 2024 2:07 PM > > To: Ratheesh Kannoth <rkannoth@marvell.com> > > Cc: netdev@vger.kernel.org > > Subject: [EXTERNAL] Re: [PATCH iproute2-next v2] arpd: create /var/lib/arpd > > on first use > > > > > + if (strcmp(default_dbname, dbname) == 0 > > > > + && mkdir(ARPDDIR, 0755) != 0 > > > > + && errno != EEXIST > > > why do you need errno != EEXIST case ? mkdir() will return error in this case > > as well. > > > > EEXIST is not an error in this case: if the default location already exist, all is > > good. mkdir would still return -1 in this case, so we need to exclude it > > manually. > > ACK. IMO, it would make a more readable code if you consider splitting the "if" loop. Something like this ? I tend to pack conditions unless branching is necessary, but no problem if this form is preferred. if (strcmp(default_dbname, dbname) == 0) { if (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"); > > > > -- > > > > 2.44.0 > > > > > > > > -- > > Max Gautier
> From: Max Gautier <mg@max.gautier.name> > Sent: Monday, March 18, 2024 2:29 PM > To: Ratheesh Kannoth <rkannoth@marvell.com> > Cc: netdev@vger.kernel.org > Subject: Re: [EXTERNAL] Re: [PATCH iproute2-next v2] arpd: create > /var/lib/arpd on first use > > On Mon, Mar 18, 2024 at 08:51:40AM +0000, Ratheesh Kannoth wrote: > > > From: Max Gautier <mg@max.gautier.name> > > > Sent: Monday, March 18, 2024 2:07 PM > > > To: Ratheesh Kannoth <rkannoth@marvell.com> > > > Cc: netdev@vger.kernel.org > > > Subject: [EXTERNAL] Re: [PATCH iproute2-next v2] arpd: create > > > /var/lib/arpd on first use > > > > > > > + if (strcmp(default_dbname, dbname) == 0 > > > > > + && mkdir(ARPDDIR, 0755) != 0 > > > > > + && errno != EEXIST > > > > why do you need errno != EEXIST case ? mkdir() will return error > > > > in this case > > > as well. > > > > > > EEXIST is not an error in this case: if the default location already > > > exist, all is good. mkdir would still return -1 in this case, so we > > > need to exclude it manually. > > > > ACK. IMO, it would make a more readable code if you consider splitting the > "if" loop. > > Something like this ? I tend to pack conditions unless branching is necessary, > but no problem if this form is preferred. > > if (strcmp(default_dbname, dbname) == 0) { > if (mkdir(ARPDDIR, 0755) != 0 && errno != EEXIST) { > ... > } > } ACK. instead of errno != EXIST , you may consider stat() before mkdir() call. Just my way thinking(please ignore it, if you don't like). My thinking is --> you need to execute mkdir () only first time, second time onwards, stat() call will return 0.
On Mon, Mar 18, 2024 at 09:18:59AM +0000, Ratheesh Kannoth wrote: > > > > > > + if (strcmp(default_dbname, dbname) == 0 > > > > > > + && mkdir(ARPDDIR, 0755) != 0 > > > > > > + && errno != EEXIST > > > > > why do you need errno != EEXIST case ? mkdir() will return error > > > > > in this case > > > > as well. > > > > > > > > EEXIST is not an error in this case: if the default location already > > > > exist, all is good. mkdir would still return -1 in this case, so we > > > > need to exclude it manually. > > > > > > ACK. IMO, it would make a more readable code if you consider splitting the > > "if" loop. > > > > Something like this ? I tend to pack conditions unless branching is necessary, > > but no problem if this form is preferred. > > > > if (strcmp(default_dbname, dbname) == 0) { > > if (mkdir(ARPDDIR, 0755) != 0 && errno != EEXIST) { > > ... > > } > > } > ACK. > instead of errno != EXIST , you may consider stat() before mkdir() call. Just my way thinking(please ignore it, if you don't like). > My thinking is --> you need to execute mkdir () only first time, second time onwards, stat() call will return 0. That's racy: we can stat and have a non existing folder, then have another arpd instance (or anything else, really) create the directory, and we would hit EEXIST anyway when we call mkdir. Also, that needs two syscalls instead of one.
On 3/18/24 12:26, Max Gautier wrote: > On Mon, Mar 18, 2024 at 09:18:59AM +0000, Ratheesh Kannoth wrote: >>>>>>> + if (strcmp(default_dbname, dbname) == 0 >>>>>>> + && mkdir(ARPDDIR, 0755) != 0 >>>>>>> + && errno != EEXIST >>>>>> why do you need errno != EEXIST case ? mkdir() will return error >>>>>> in this case >>>>> as well. >>>>> >>>>> EEXIST is not an error in this case: if the default location already >>>>> exist, all is good. mkdir would still return -1 in this case, so we >>>>> need to exclude it manually. >>>> >>>> ACK. IMO, it would make a more readable code if you consider splitting the >>> "if" loop. >>> >>> Something like this ? I tend to pack conditions unless branching is necessary, >>> but no problem if this form is preferred. >>> >>> if (strcmp(default_dbname, dbname) == 0) { >>> if (mkdir(ARPDDIR, 0755) != 0 && errno != EEXIST) { >>> ... >>> } >>> } >> ACK. >> instead of errno != EXIST , you may consider stat() before mkdir() call. Just my way thinking(please ignore it, if you don't like). >> My thinking is --> you need to execute mkdir () only first time, second time onwards, stat() call will return 0. > > That's racy: we can stat and have a non existing folder, then have > another arpd instance (or anything else, really) create the directory, Agreed ^^ > and we would hit EEXIST anyway when we call mkdir. > Also, that needs two syscalls instead of one. >
> From: Max Gautier <mg@max.gautier.name> > Sent: Monday, March 18, 2024 2:57 PM > To: Ratheesh Kannoth <rkannoth@marvell.com> > Cc: netdev@vger.kernel.org > Subject: Re: [EXTERNAL] Re: [PATCH iproute2-next v2] arpd: create > /var/lib/arpd on first use > > On Mon, Mar 18, 2024 at 09:18:59AM +0000, Ratheesh Kannoth wrote: > > > > > > > + if (strcmp(default_dbname, dbname) == 0 > > > > > > > + && mkdir(ARPDDIR, 0755) != 0 > > > > > > > + && errno != EEXIST > > > > > > why do you need errno != EEXIST case ? mkdir() will return > > > > > > error in this case > > > > > as well. > > > > > > > > > > EEXIST is not an error in this case: if the default location > > > > > already exist, all is good. mkdir would still return -1 in this > > > > > case, so we need to exclude it manually. > > > > > > > > ACK. IMO, it would make a more readable code if you consider > > > > splitting the > > > "if" loop. > > > > > > Something like this ? I tend to pack conditions unless branching is > > > necessary, but no problem if this form is preferred. > > > > > > if (strcmp(default_dbname, dbname) == 0) { > > > if (mkdir(ARPDDIR, 0755) != 0 && errno != EEXIST) { > > > ... > > > } > > > } > > ACK. > > instead of errno != EXIST , you may consider stat() before mkdir() call. Just > my way thinking(please ignore it, if you don't like). > > My thinking is --> you need to execute mkdir () only first time, second time > onwards, stat() call will return 0. > > That's racy: we can stat and have a non existing folder, then have another arpd > instance (or anything else, really) create the directory, and we would hit EEXIST > anyway when we call mkdir. ACK. > Also, that needs two syscalls instead of one. > > -- > Max Gautier
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..a64888aa 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 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 (strcmp(default_dbname, dbname) == 0 + && 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");
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 Signed-off-by: Max Gautier <mg@max.gautier.name> --- Makefile | 2 +- misc/arpd.c | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-)