diff mbox series

[for-4.17] tools/ocaml/xenstored: fix live update exception

Message ID 12d90632bf881e96e0b6c256df193f00df187dc1.1666284745.git.edvin.torok@citrix.com (mailing list archive)
State Superseded
Headers show
Series [for-4.17] tools/ocaml/xenstored: fix live update exception | expand

Commit Message

Edwin Török Oct. 20, 2022, 4:54 p.m. UTC 
During live update we will load the /tool/xenstored path from the previous binary,
and then try to mkdir /tool again which will fail with EEXIST.
Check for existence of the path before creating it.

The write call to /tool/xenstored should not need any changes
(and we do want to overwrite any previous path, in case it changed).

Prior to 7110192b1df6 live update would work only if the binary path was
specified, and with 7110192b1df6 and this live update also works when
no binary path is specified in `xenstore-control live-update`.

Fixes: 7110192b1df6 ("tools/oxenstored: Fix Oxenstored Live Update")
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/xenstored/xenstored.ml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


base-commit: 0c06760be3dc3f286015e18c4b1d1694e55da026

Comments

Henry Wang Oct. 21, 2022, 1:51 a.m. UTC | #1 
Hi Edwin,

> -----Original Message-----
> From: Edwin Török <edvin.torok@citrix.com>
> Subject: [PATCH for-4.17] tools/ocaml/xenstored: fix live update exception
> 
> During live update we will load the /tool/xenstored path from the previous
> binary,
> and then try to mkdir /tool again which will fail with EEXIST.
> Check for existence of the path before creating it.
> 
> The write call to /tool/xenstored should not need any changes
> (and we do want to overwrite any previous path, in case it changed).
> 
> Prior to 7110192b1df6 live update would work only if the binary path was
> specified, and with 7110192b1df6 and this live update also works when
> no binary path is specified in `xenstore-control live-update`.
> 
> Fixes: 7110192b1df6 ("tools/oxenstored: Fix Oxenstored Live Update")
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>

As per [1] described, I think it is quite reasonable to include this patch for
4.17, as this patch is a bugfix and the patch in [1] already have the ack and
release ack. Although I would also like to hear the opinion from
Christian (the ocaml maintainer) to just play safe.

[1] https://lore.kernel.org/xen-devel/F65C8E58-EF3D-47D9-A94E-7B70EB93E068@citrix.com/

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> ---
>  tools/ocaml/xenstored/xenstored.ml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/ocaml/xenstored/xenstored.ml
> b/tools/ocaml/xenstored/xenstored.ml
> index fc90fcdeb5..3299fe73f7 100644
> --- a/tools/ocaml/xenstored/xenstored.ml
> +++ b/tools/ocaml/xenstored/xenstored.ml
> @@ -353,7 +353,9 @@ let _ =
>  	) in
> 
>  	(* required for xenstore-control to detect availability of live-update *)
> -	Store.mkdir store Perms.Connection.full_rights (Store.Path.of_string
> "/tool");
> +	let tool_path = Store.Path.of_string "/tool" in
> +	if not (Store.path_exists store tool_path) then
> +					Store.mkdir store
> Perms.Connection.full_rights tool_path;
>  	Store.write store Perms.Connection.full_rights
>  		(Store.Path.of_string "/tool/xenstored")
> Sys.executable_name;
> 
> 
> base-commit: 0c06760be3dc3f286015e18c4b1d1694e55da026
> --
> 2.34.1
Christian Lindig Oct. 21, 2022, 7:50 a.m. UTC | #2 
> On 20 Oct 2022, at 17:54, Edwin Török <edvin.torok@citrix.com> wrote:
> 
> During live update we will load the /tool/xenstored path from the previous binary,
> and then try to mkdir /tool again which will fail with EEXIST.
> Check for existence of the path before creating it.
> 
> The write call to /tool/xenstored should not need any changes
> (and we do want to overwrite any previous path, in case it changed).
> 
> Prior to 7110192b1df6 live update would work only if the binary path was
> specified, and with 7110192b1df6 and this live update also works when
> no binary path is specified in `xenstore-control live-update`.
> 
> Fixes: 7110192b1df6 ("tools/oxenstored: Fix Oxenstored Live Update")
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
> ---
> tools/ocaml/xenstored/xenstored.ml | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
> index fc90fcdeb5..3299fe73f7 100644
> --- a/tools/ocaml/xenstored/xenstored.ml
> +++ b/tools/ocaml/xenstored/xenstored.ml
> @@ -353,7 +353,9 @@ let _ =
> 	) in
> 
> 	(* required for xenstore-control to detect availability of live-update *)
> -	Store.mkdir store Perms.Connection.full_rights (Store.Path.of_string "/tool");
> +	let tool_path = Store.Path.of_string "/tool" in
> +	if not (Store.path_exists store tool_path) then
> +					Store.mkdir store Perms.Connection.full_rights tool_path;
> 	Store.write store Perms.Connection.full_rights
> 		(Store.Path.of_string "/tool/xenstored") Sys.executable_name;

I notice inconsistent indentation but let's ignore that or fix it before the committing.

Acked-by: Christian Lindig <christian.lindig@citrix.com>
Edwin Török Oct. 21, 2022, 8:01 a.m. UTC | #3 
> On 21 Oct 2022, at 08:50, Christian Lindig <christian.lindig@citrix.com> wrote:
> 
> 
> 
>> On 20 Oct 2022, at 17:54, Edwin Török <edvin.torok@citrix.com> wrote:
>> 
>> During live update we will load the /tool/xenstored path from the previous binary,
>> and then try to mkdir /tool again which will fail with EEXIST.
>> Check for existence of the path before creating it.
>> 
>> The write call to /tool/xenstored should not need any changes
>> (and we do want to overwrite any previous path, in case it changed).
>> 
>> Prior to 7110192b1df6 live update would work only if the binary path was
>> specified, and with 7110192b1df6 and this live update also works when
>> no binary path is specified in `xenstore-control live-update`.
>> 
>> Fixes: 7110192b1df6 ("tools/oxenstored: Fix Oxenstored Live Update")
>> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
>> ---
>> tools/ocaml/xenstored/xenstored.ml | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
>> index fc90fcdeb5..3299fe73f7 100644
>> --- a/tools/ocaml/xenstored/xenstored.ml
>> +++ b/tools/ocaml/xenstored/xenstored.ml
>> @@ -353,7 +353,9 @@ let _ =
>> 	) in
>> 
>> 	(* required for xenstore-control to detect availability of live-update *)
>> -	Store.mkdir store Perms.Connection.full_rights (Store.Path.of_string "/tool");
>> +	let tool_path = Store.Path.of_string "/tool" in
>> +	if not (Store.path_exists store tool_path) then
>> +					Store.mkdir store Perms.Connection.full_rights tool_path;
>> 	Store.write store Perms.Connection.full_rights
>> 		(Store.Path.of_string "/tool/xenstored") Sys.executable_name;
> 
> I notice inconsistent indentation but let's ignore that or fix it before the committing.
> 
> Acked-by: Christian Lindig <christian.lindig@citrix.com>
> 


Thanks, fixed indentation here: https://github.com/edwintorok/xen/commit/4a89f1f44cb171e1f92dae2401a580a10fd0c5a0
And v2 patch should show up on the ML with the 2 acks included and fixed indentation soon too.

Best regards,
--Edwin
diff mbox series

Patch

diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index fc90fcdeb5..3299fe73f7 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -353,7 +353,9 @@  let _ =
 	) in
 
 	(* required for xenstore-control to detect availability of live-update *)
-	Store.mkdir store Perms.Connection.full_rights (Store.Path.of_string "/tool");
+	let tool_path = Store.Path.of_string "/tool" in
+	if not (Store.path_exists store tool_path) then
+					Store.mkdir store Perms.Connection.full_rights tool_path;
 	Store.write store Perms.Connection.full_rights
 		(Store.Path.of_string "/tool/xenstored") Sys.executable_name;