Re: [PATCH v2 3/6] tools/oxenstored: Rename some 'port' variables to 'remote_port'

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Christian Lindig <christian.lindig@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	David Scott <dave@recoil.org>,
	Edwin Torok <edvin.torok@citrix.com>,
	Rob Hoes <Rob.Hoes@citrix.com>
Subject: Re: [PATCH v2 3/6] tools/oxenstored: Rename some 'port' variables to 'remote_port'
Date: Thu, 1 Dec 2022 12:02:54 +0000	[thread overview]
Message-ID: <240e9bf1-3f10-7c65-8e8d-cb192359919d@citrix.com> (raw)
In-Reply-To: <775FA3EA-6F85-4706-8159-EB8CFFD983E2@citrix.com>

On 01/12/2022 11:26, Christian Lindig wrote:
>> On 30 Nov 2022, at 16:54, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>
>> This will make the logic clearer when we plumb local_port through these
>> functions.
>>
>> While changing this, simplify the construct in Domains.create0 to separate the
>> remote port handling from the interface.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Christian Lindig <christian.lindig@citrix.com>
>> CC: David Scott <dave@recoil.org>
>> CC: Edwin Torok <edvin.torok@citrix.com>
>> CC: Rob Hoes <Rob.Hoes@citrix.com>
> Acked-by: Christian Lindig <christian.lindig@citrix.com>

Thanks.

>> diff --git a/tools/ocaml/xenstored/domains.ml b/tools/ocaml/xenstored/domains.ml
>> index 17fe2fa25772..26018ac0dd3d 100644
>> --- a/tools/ocaml/xenstored/domains.ml
>> +++ b/tools/ocaml/xenstored/domains.ml
>> @@ -133,18 +133,16 @@ let xenstored_kva = ref ""
>> let xenstored_port = ref ""
>>
>> let create0 doms =
>> -	let port, interface =
>> -		(
>> -			let port = Utils.read_file_single_integer !xenstored_port
>> -			and fd = Unix.openfile !xenstored_kva
>> -					       [ Unix.O_RDWR ] 0o600 in
>> -			let interface = Xenmmap.mmap fd Xenmmap.RDWR Xenmmap.SHARED
>> -						  (Xenmmap.getpagesize()) 0 in
>> -			Unix.close fd;
>> -			port, interface
>> -		)
>> -		in
>> -	let dom = Domain.make 0 Nativeint.zero port interface doms.eventchn in
>> +	let remote_port = Utils.read_file_single_integer !xenstored_port in
>> +
>> +	let interface =
>> +		let fd = Unix.openfile !xenstored_kva [ Unix.O_RDWR ] 0o600 in
>> +		let interface = Xenmmap.mmap fd Xenmmap.RDWR Xenmmap.SHARED (Xenmmap.getpagesize()) 0 in
> Can we be sure that this never throws an exception such that the close can't be missed? Otherwise a Fun.protect (or equivalent) should be used.

This mess also depends on !xenstored_port and !xenstored_kva morphing
into something other than ""  before Domain.create0 is called.

But this logic is also the penultimate unstable ABI in oxenstored, and
will be removed fully when we can bind /dev/xen/gntdev for Ocaml and
replace the foreign mapping with "map grant 1" (also removing this as a
special case difference between dom0 and all other domains.)

So I'm tempted to argue that I'm not actually changing the behaviour
here, and it's not worth fixing up logic this fragile when we're
intending to replace it anyway.  Edvin has patches IIRC, but they need
rebasing.

~Andrew