1. Patchew
  2. Xen
  3. [PATCH 0/3] tools/hotplug: Fixes to vif-nat
  4. View patch

[PATCH 2/3] tools/hotplug: Fix dhcpd symlink removal in vif-nat

Diego Sueiro posted 3 patches 5 years, 5 months ago
Maintainers: Wei Liu <wl@xen.org>, Ian Jackson <ian.jackson@eu.citrix.com>
[PATCH 2/3] tools/hotplug: Fix dhcpd symlink removal in vif-nat
Posted by Diego Sueiro 5 years, 5 months ago 
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(-)

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
Re: [PATCH 2/3] tools/hotplug: Fix dhcpd symlink removal in vif-nat
Posted by Bertrand Marquis 5 years, 5 months ago 

> 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
> 
>
Re: [PATCH 2/3] tools/hotplug: Fix dhcpd symlink removal in vif-nat
Posted by Wei Liu 5 years, 5 months ago 
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
>
Re: [PATCH 2/3] tools/hotplug: Fix dhcpd symlink removal in vif-nat
Posted by Bertrand Marquis 5 years, 5 months ago 
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
Re: [PATCH 2/3] tools/hotplug: Fix dhcpd symlink removal in vif-nat
Posted by Wei Liu 5 years, 5 months ago 
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.
RE: [PATCH 2/3] tools/hotplug: Fix dhcpd symlink removal in vif-nat
Posted by Diego Sueiro 5 years, 5 months ago 
>-----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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.