[PATCH 0/3] tools/hotplug: Fixes to vif-nat

[PATCH 0/3] tools/hotplug: Fixes to vif-nat

[Diego Sueiro]

This patch series fixes issues around the vif-nat script when
setting up the vif interface and dhcp server in dom0.

It has been validated and used in Yocto and meta-arm-autonomy

Diego Sueiro (3):
  tools/hotplug: Fix hostname setting in vif-nat
  tools/hotplug: Fix dhcpd symlink removal in vif-nat
  tools/hotplug: Extend dhcpd conf, init and arg files search

 tools/hotplug/Linux/vif-nat               | 14 ++++++++------
 tools/hotplug/Linux/xen-network-common.sh |  6 +++---
 2 files changed, 11 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH 1/3] tools/hotplug: Fix hostname setting in vif-nat

[Diego Sueiro]

Setting the hostname is failing because the "$XENBUS_PATH/domain"
doesn't exist anymore. To fix this we set it to dom$domid

Signed-off-by: Diego Sueiro <diego.sueiro@arm.com>
---
 tools/hotplug/Linux/vif-nat | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/hotplug/Linux/vif-nat b/tools/hotplug/Linux/vif-nat
index a76d9c7..2614435 100644
--- a/tools/hotplug/Linux/vif-nat
+++ b/tools/hotplug/Linux/vif-nat
@@ -85,7 +85,7 @@ router_ip=$(routing_ip "$ip")
 # Split the given IP/bits pair.
 vif_ip=`echo ${ip} | awk -F/ '{print $1}'`
 
-hostname=$(xenstore_read "$XENBUS_PATH/domain" | tr -- '_.:/+' '-----')
+hostname=dom$domid
 if [ "$vifid" != "1" ]
 then
   hostname="$hostname-$vifid"
-- 
2.7.4

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

[Diego Sueiro]

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

[PATCH 3/3] tools/hotplug: Extend dhcpd conf, init and arg files search

[Diego Sueiro]

Newer versions of the ISC dhcp server expect the dhcpd.conf file
to be located at /etc/dhcp directory.

Also, some distributions and Yocto based ones have these installation
paths by default: /etc/init.d/{isc-dhcp-server,dhcp-server} and
/etc/default/dhcp-server.

Signed-off-by: Diego Sueiro <diego.sueiro@arm.com>
---
 tools/hotplug/Linux/xen-network-common.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 8dd3a62..be632ce 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -64,18 +64,18 @@ first_file()
 
 find_dhcpd_conf_file()
 {
-  first_file -f /etc/dhcp3/dhcpd.conf /etc/dhcpd.conf
+  first_file -f /etc/dhcp/dhcpd.conf /etc/dhcp3/dhcpd.conf /etc/dhcpd.conf
 }
 
 
 find_dhcpd_init_file()
 {
-  first_file -x /etc/init.d/{dhcp3-server,dhcp,dhcpd}
+  first_file -x /etc/init.d/{isc-dhcp-server,dhcp-server,dhcp3-server,dhcp,dhcpd}
 }
 
 find_dhcpd_arg_file()
 {
-  first_file -f /etc/sysconfig/dhcpd /etc/defaults/dhcp /etc/default/dhcp3-server
+  first_file -f /etc/sysconfig/dhcpd /etc/defaults/dhcp /etc/default/dhcp-server /etc/default/dhcp3-server
 }
 
 # configure interfaces which act as pure bridge ports:
-- 
2.7.4

Re: [PATCH 1/3] tools/hotplug: Fix hostname setting in vif-nat

[Bertrand Marquis]

> On 20 Aug 2020, at 11:58, Diego Sueiro <Diego.Sueiro@arm.com> wrote:
> 
> Setting the hostname is failing because the "$XENBUS_PATH/domain"
> doesn't exist anymore. To fix this we set it to dom$domid
> 
> Signed-off-by: Diego Sueiro <diego.sueiro@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

> ---
> tools/hotplug/Linux/vif-nat | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/hotplug/Linux/vif-nat b/tools/hotplug/Linux/vif-nat
> index a76d9c7..2614435 100644
> --- a/tools/hotplug/Linux/vif-nat
> +++ b/tools/hotplug/Linux/vif-nat
> @@ -85,7 +85,7 @@ router_ip=$(routing_ip "$ip")
> # Split the given IP/bits pair.
> vif_ip=`echo ${ip} | awk -F/ '{print $1}'`
> 
> -hostname=$(xenstore_read "$XENBUS_PATH/domain" | tr -- '_.:/+' '-----')
> +hostname=dom$domid
> if [ "$vifid" != "1" ]
> then
>   hostname="$hostname-$vifid"
> -- 
> 2.7.4
> 
> 

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

[Bertrand Marquis]

> 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 3/3] tools/hotplug: Extend dhcpd conf, init and arg files search

[Bertrand Marquis]

> On 20 Aug 2020, at 12:01, Diego Sueiro <Diego.Sueiro@arm.com> wrote:
> 
> Newer versions of the ISC dhcp server expect the dhcpd.conf file
> to be located at /etc/dhcp directory.
> 
> Also, some distributions and Yocto based ones have these installation
> paths by default: /etc/init.d/{isc-dhcp-server,dhcp-server} and
> /etc/default/dhcp-server.
> 
> Signed-off-by: Diego Sueiro <diego.sueiro@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

> ---
> tools/hotplug/Linux/xen-network-common.sh | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
> index 8dd3a62..be632ce 100644
> --- a/tools/hotplug/Linux/xen-network-common.sh
> +++ b/tools/hotplug/Linux/xen-network-common.sh
> @@ -64,18 +64,18 @@ first_file()
> 
> find_dhcpd_conf_file()
> {
> -  first_file -f /etc/dhcp3/dhcpd.conf /etc/dhcpd.conf
> +  first_file -f /etc/dhcp/dhcpd.conf /etc/dhcp3/dhcpd.conf /etc/dhcpd.conf
> }
> 
> 
> find_dhcpd_init_file()
> {
> -  first_file -x /etc/init.d/{dhcp3-server,dhcp,dhcpd}
> +  first_file -x /etc/init.d/{isc-dhcp-server,dhcp-server,dhcp3-server,dhcp,dhcpd}
> }
> 
> find_dhcpd_arg_file()
> {
> -  first_file -f /etc/sysconfig/dhcpd /etc/defaults/dhcp /etc/default/dhcp3-server
> +  first_file -f /etc/sysconfig/dhcpd /etc/defaults/dhcp /etc/default/dhcp-server /etc/default/dhcp3-server
> }
> 
> # configure interfaces which act as pure bridge ports:
> -- 
> 2.7.4
> 
> 

Re: [PATCH 0/3] tools/hotplug: Fixes to vif-nat

[Bertrand Marquis]

Hi,

A gentle ping to have a review/ack on this serie :-)

(Adding some people in CC)

Bertrand

> On 20 Aug 2020, at 11:46, Diego Sueiro <Diego.Sueiro@arm.com> wrote:
> 
> This patch series fixes issues around the vif-nat script when
> setting up the vif interface and dhcp server in dom0.
> 
> It has been validated and used in Yocto and meta-arm-autonomy
> 
> Diego Sueiro (3):
>  tools/hotplug: Fix hostname setting in vif-nat
>  tools/hotplug: Fix dhcpd symlink removal in vif-nat
>  tools/hotplug: Extend dhcpd conf, init and arg files search
> 
> tools/hotplug/Linux/vif-nat               | 14 ++++++++------
> tools/hotplug/Linux/xen-network-common.sh |  6 +++---
> 2 files changed, 11 insertions(+), 9 deletions(-)
> 
> -- 
> 2.7.4
> 
> 

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

[Wei Liu]

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 1/3] tools/hotplug: Fix hostname setting in vif-nat

[Wei Liu]

On Thu, Aug 27, 2020 at 02:14:03PM +0000, Bertrand Marquis wrote:
> 
> 
> > On 20 Aug 2020, at 11:58, Diego Sueiro <Diego.Sueiro@arm.com> wrote:
> > 
> > Setting the hostname is failing because the "$XENBUS_PATH/domain"
> > doesn't exist anymore. To fix this we set it to dom$domid
> > 
> > Signed-off-by: Diego Sueiro <diego.sueiro@arm.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 

Acked-by: Wei Liu <wl@xen.org>

Re: [PATCH 3/3] tools/hotplug: Extend dhcpd conf, init and arg files search

[Wei Liu]

On Thu, Aug 20, 2020 at 12:01:11PM +0100, Diego Sueiro wrote:
> Newer versions of the ISC dhcp server expect the dhcpd.conf file
> to be located at /etc/dhcp directory.
> 
> Also, some distributions and Yocto based ones have these installation
> paths by default: /etc/init.d/{isc-dhcp-server,dhcp-server} and
> /etc/default/dhcp-server.
> 
> Signed-off-by: Diego Sueiro <diego.sueiro@arm.com>

Acked-by: Wei Liu <wl@xen.org>

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

[Bertrand Marquis]

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

[Wei Liu]

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

[Diego Sueiro]

>-----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.