| Message ID | 3b0efb9fb1ba94922c0ae156c0ab0be6a9f45f25.1597920095.git.diego.sueiro@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/hotplug: Fixes to vif-nat | expand |
> On 20 Aug 2020, at 12:00, Diego Sueiro <Diego.Sueiro@arm.com> wrote: > > Copy temp files used to add/remove dhcpd configurations to avoid > replacing potential symlinks. > > Signed-off-by: Diego Sueiro <diego.sueiro@arm.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > --- > tools/hotplug/Linux/vif-nat | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/tools/hotplug/Linux/vif-nat b/tools/hotplug/Linux/vif-nat > index 2614435..1ab80ed 100644 > --- a/tools/hotplug/Linux/vif-nat > +++ b/tools/hotplug/Linux/vif-nat > @@ -99,7 +99,8 @@ dhcparg_remove_entry() > then > rm "$tmpfile" > else > - mv "$tmpfile" "$dhcpd_arg_file" > + cp "$tmpfile" "$dhcpd_arg_file" > + rm "$tmpfile" > fi > } > > @@ -109,11 +110,11 @@ dhcparg_add_entry() > local tmpfile=$(mktemp) > # handle Red Hat, SUSE, and Debian styles, with or without quotes > sed -e 's/^DHCPDARGS="*\([^"]*\)"*/DHCPDARGS="\1'"${dev} "'"/' \ > - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" > + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" > sed -e 's/^DHCPD_INTERFACE="*\([^"]*\)"*/DHCPD_INTERFACE="\1'"${dev} "'"/' \ > - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" > + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" > sed -e 's/^INTERFACES="*\([^"]*\)"*/INTERFACES="\1'"${dev} "'"/' \ > - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" > + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" > rm -f "$tmpfile" > } > > @@ -125,7 +126,8 @@ dhcp_remove_entry() > then > rm "$tmpfile" > else > - mv "$tmpfile" "$dhcpd_conf_file" > + cp "$tmpfile" "$dhcpd_conf_file" > + rm "$tmpfile" > fi > dhcparg_remove_entry > } > -- > 2.7.4 > >
On Thu, Aug 20, 2020 at 12:00:23PM +0100, Diego Sueiro wrote: > Copy temp files used to add/remove dhcpd configurations to avoid > replacing potential symlinks. > Can you clarify the issue you saw a bit? Which one of the parameter is a symlink (I assume the latter) and what problem you see with replacing the symlinks? > Signed-off-by: Diego Sueiro <diego.sueiro@arm.com> > --- > tools/hotplug/Linux/vif-nat | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/tools/hotplug/Linux/vif-nat b/tools/hotplug/Linux/vif-nat > index 2614435..1ab80ed 100644 > --- a/tools/hotplug/Linux/vif-nat > +++ b/tools/hotplug/Linux/vif-nat > @@ -99,7 +99,8 @@ dhcparg_remove_entry() > then > rm "$tmpfile" > else > - mv "$tmpfile" "$dhcpd_arg_file" > + cp "$tmpfile" "$dhcpd_arg_file" > + rm "$tmpfile" > fi You could've simplified the code a bit here and below now that both branches issue the same rm command. But don't resend just yet. Please help me understand your issue first. Wei. > } > > @@ -109,11 +110,11 @@ dhcparg_add_entry() > local tmpfile=$(mktemp) > # handle Red Hat, SUSE, and Debian styles, with or without quotes > sed -e 's/^DHCPDARGS="*\([^"]*\)"*/DHCPDARGS="\1'"${dev} "'"/' \ > - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" > + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" > sed -e 's/^DHCPD_INTERFACE="*\([^"]*\)"*/DHCPD_INTERFACE="\1'"${dev} "'"/' \ > - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" > + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" > sed -e 's/^INTERFACES="*\([^"]*\)"*/INTERFACES="\1'"${dev} "'"/' \ > - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" > + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" > rm -f "$tmpfile" > } > > @@ -125,7 +126,8 @@ dhcp_remove_entry() > then > rm "$tmpfile" > else > - mv "$tmpfile" "$dhcpd_conf_file" > + cp "$tmpfile" "$dhcpd_conf_file" > + rm "$tmpfile" > fi > dhcparg_remove_entry > } > -- > 2.7.4 >
Hi Wei, > On 7 Sep 2020, at 15:36, Wei Liu <wl@xen.org> wrote: > > On Thu, Aug 20, 2020 at 12:00:23PM +0100, Diego Sueiro wrote: >> Copy temp files used to add/remove dhcpd configurations to avoid >> replacing potential symlinks. >> > > Can you clarify the issue you saw a bit? > > Which one of the parameter is a symlink (I assume the latter) and what > problem you see with replacing the symlinks? Maybe i can explain here. If you have this: /etc/dhcp.conf -> dhcp.conf.real mv will create a new file dhcp.conf where cp will actually modify dhcp.conf.real instead of replacing the symlink with a real file. This prevents some mistakes where the user will actually continue to modify dhcp.conf.real where it would not be the one used anymore. Bertrand > >> Signed-off-by: Diego Sueiro <diego.sueiro@arm.com> >> --- >> tools/hotplug/Linux/vif-nat | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/tools/hotplug/Linux/vif-nat b/tools/hotplug/Linux/vif-nat >> index 2614435..1ab80ed 100644 >> --- a/tools/hotplug/Linux/vif-nat >> +++ b/tools/hotplug/Linux/vif-nat >> @@ -99,7 +99,8 @@ dhcparg_remove_entry() >> then >> rm "$tmpfile" >> else >> - mv "$tmpfile" "$dhcpd_arg_file" >> + cp "$tmpfile" "$dhcpd_arg_file" >> + rm "$tmpfile" >> fi > > You could've simplified the code a bit here and below now that both > branches issue the same rm command. > > But don't resend just yet. Please help me understand your issue first. > > Wei. > >> } >> >> @@ -109,11 +110,11 @@ dhcparg_add_entry() >> local tmpfile=$(mktemp) >> # handle Red Hat, SUSE, and Debian styles, with or without quotes >> sed -e 's/^DHCPDARGS="*\([^"]*\)"*/DHCPDARGS="\1'"${dev} "'"/' \ >> - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" >> + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" >> sed -e 's/^DHCPD_INTERFACE="*\([^"]*\)"*/DHCPD_INTERFACE="\1'"${dev} "'"/' \ >> - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" >> + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" >> sed -e 's/^INTERFACES="*\([^"]*\)"*/INTERFACES="\1'"${dev} "'"/' \ >> - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" >> + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" >> rm -f "$tmpfile" >> } >> >> @@ -125,7 +126,8 @@ dhcp_remove_entry() >> then >> rm "$tmpfile" >> else >> - mv "$tmpfile" "$dhcpd_conf_file" >> + cp "$tmpfile" "$dhcpd_conf_file" >> + rm "$tmpfile" >> fi >> dhcparg_remove_entry >> } >> -- >> 2.7.4
On Mon, Sep 07, 2020 at 03:01:37PM +0000, Bertrand Marquis wrote: > Hi Wei, > > > On 7 Sep 2020, at 15:36, Wei Liu <wl@xen.org> wrote: > > > > On Thu, Aug 20, 2020 at 12:00:23PM +0100, Diego Sueiro wrote: > >> Copy temp files used to add/remove dhcpd configurations to avoid > >> replacing potential symlinks. > >> > > > > Can you clarify the issue you saw a bit? > > > > Which one of the parameter is a symlink (I assume the latter) and what > > problem you see with replacing the symlinks? > > Maybe i can explain here. > > If you have this: > /etc/dhcp.conf -> dhcp.conf.real > > mv will create a new file dhcp.conf where cp will actually modify > dhcp.conf.real instead of replacing the symlink with a real file. > > This prevents some mistakes where the user will actually continue to > modify dhcp.conf.real where it would not be the one used anymore. OK. Now I understand the use case. Thanks. I think you explanation should be part of the commit message. Diego, can you please incorporate Bertrand's explanation and deal with my comment below? > >> --- > >> tools/hotplug/Linux/vif-nat | 12 +++++++----- > >> 1 file changed, 7 insertions(+), 5 deletions(-) > >> > >> diff --git a/tools/hotplug/Linux/vif-nat b/tools/hotplug/Linux/vif-nat > >> index 2614435..1ab80ed 100644 > >> --- a/tools/hotplug/Linux/vif-nat > >> +++ b/tools/hotplug/Linux/vif-nat > >> @@ -99,7 +99,8 @@ dhcparg_remove_entry() > >> then > >> rm "$tmpfile" > >> else > >> - mv "$tmpfile" "$dhcpd_arg_file" > >> + cp "$tmpfile" "$dhcpd_arg_file" > >> + rm "$tmpfile" > >> fi > > > > You could've simplified the code a bit here and below now that both > > branches issue the same rm command. Wei.
>-----Original Message----- >From: Wei Liu <wl@xen.org> >Sent: 07 September 2020 16:10 >To: Bertrand Marquis <Bertrand.Marquis@arm.com> >Cc: Wei Liu <wl@xen.org>; Diego Sueiro <Diego.Sueiro@arm.com>; xen- >devel@lists.xenproject.org; nd <nd@arm.com>; Ian Jackson ><ian.jackson@eu.citrix.com> >Subject: Re: [PATCH 2/3] tools/hotplug: Fix dhcpd symlink removal in vif-nat > >On Mon, Sep 07, 2020 at 03:01:37PM +0000, Bertrand Marquis wrote: >> Hi Wei, >> >> > On 7 Sep 2020, at 15:36, Wei Liu <wl@xen.org> wrote: >> > >> > On Thu, Aug 20, 2020 at 12:00:23PM +0100, Diego Sueiro wrote: >> >> Copy temp files used to add/remove dhcpd configurations to avoid >> >> replacing potential symlinks. >> >> >> > >> > Can you clarify the issue you saw a bit? >> > >> > Which one of the parameter is a symlink (I assume the latter) and >> > what problem you see with replacing the symlinks? >> >> Maybe i can explain here. >> >> If you have this: >> /etc/dhcp.conf -> dhcp.conf.real >> >> mv will create a new file dhcp.conf where cp will actually modify >> dhcp.conf.real instead of replacing the symlink with a real file. >> >> This prevents some mistakes where the user will actually continue to >> modify dhcp.conf.real where it would not be the one used anymore. > >OK. Now I understand the use case. Thanks. > >I think you explanation should be part of the commit message. > >Diego, can you please incorporate Bertrand's explanation and deal with my >comment below? > Done and v2 sent to the mailing list. Thanks for your review. -- Diego Sueiro >> >> --- >> >> tools/hotplug/Linux/vif-nat | 12 +++++++----- >> >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> >> >> diff --git a/tools/hotplug/Linux/vif-nat >> >> b/tools/hotplug/Linux/vif-nat index 2614435..1ab80ed 100644 >> >> --- a/tools/hotplug/Linux/vif-nat >> >> +++ b/tools/hotplug/Linux/vif-nat >> >> @@ -99,7 +99,8 @@ dhcparg_remove_entry() >> >> then >> >> rm "$tmpfile" >> >> else >> >> - mv "$tmpfile" "$dhcpd_arg_file" >> >> + cp "$tmpfile" "$dhcpd_arg_file" >> >> + rm "$tmpfile" >> >> fi >> > >> > You could've simplified the code a bit here and below now that both >> > branches issue the same rm command. > >Wei.
diff --git a/tools/hotplug/Linux/vif-nat b/tools/hotplug/Linux/vif-nat index 2614435..1ab80ed 100644 --- a/tools/hotplug/Linux/vif-nat +++ b/tools/hotplug/Linux/vif-nat @@ -99,7 +99,8 @@ dhcparg_remove_entry() then rm "$tmpfile" else - mv "$tmpfile" "$dhcpd_arg_file" + cp "$tmpfile" "$dhcpd_arg_file" + rm "$tmpfile" fi } @@ -109,11 +110,11 @@ dhcparg_add_entry() local tmpfile=$(mktemp) # handle Red Hat, SUSE, and Debian styles, with or without quotes sed -e 's/^DHCPDARGS="*\([^"]*\)"*/DHCPDARGS="\1'"${dev} "'"/' \ - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" sed -e 's/^DHCPD_INTERFACE="*\([^"]*\)"*/DHCPD_INTERFACE="\1'"${dev} "'"/' \ - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" sed -e 's/^INTERFACES="*\([^"]*\)"*/INTERFACES="\1'"${dev} "'"/' \ - "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file" + "$dhcpd_arg_file" >"$tmpfile" && cp "$tmpfile" "$dhcpd_arg_file" rm -f "$tmpfile" } @@ -125,7 +126,8 @@ dhcp_remove_entry() then rm "$tmpfile" else - mv "$tmpfile" "$dhcpd_conf_file" + cp "$tmpfile" "$dhcpd_conf_file" + rm "$tmpfile" fi dhcparg_remove_entry }
Copy temp files used to add/remove dhcpd configurations to avoid replacing potential symlinks. Signed-off-by: Diego Sueiro <diego.sueiro@arm.com> --- tools/hotplug/Linux/vif-nat | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)