| Message ID | 09afc07ba0ba65afc02028bd6b4950d8e51af69b.1667920496.git.edvin.torok@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | OCaml fixes for Xen 4.17 | expand |
Hi Edwin, > -----Original Message----- > From: Edwin Török <edvin.torok@citrix.com> > Subject: [PATCH for-4.17 v3 09/15] tools/ocaml/xenstored/store.ml: fix build > error > > Building with Dune in release mode fails with: > ``` > File "ocaml/xenstored/store.ml", line 464, characters 13-32: > Warning 18: this type-based record disambiguation is not principal. > File "ocaml/xenstored/store.ml", line 1: > Error: Some fatal warnings were triggered (1 occurrences) > ``` > > This is a warning to help keep the code futureproof, quoting from its > documentation: > > Check information path during type-checking, to make sure that all types > are > > derived in a principal way. When using labelled arguments and/or > polymorphic > > methods, this flag is required to ensure future versions of the compiler will > > be able to infer types correctly, even if internal algorithms change. All > > programs accepted in -principal mode are also accepted in the default > mode with > > equivalent types, but different binary signatures, and this may slow down > type > > checking; yet it is a good idea to use it once before publishing source code. > > Fixes: db471408edd46 "tools/ocaml/xenstored: Fix quota bypass on domain > shutdown" Nit: The format of this "Fixes:" tag might need to be fixed? > > Signed-off-by: Edwin Török <edvin.torok@citrix.com> > --- > Reason for inclusion in 4.17: > - fixes a build error in a previous commit that is already in master Yes, given this is a simple enough patch: Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
On 09.11.2022 03:47, Henry Wang wrote: >> -----Original Message----- >> From: Edwin Török <edvin.torok@citrix.com> >> Subject: [PATCH for-4.17 v3 09/15] tools/ocaml/xenstored/store.ml: fix build >> error >> >> Building with Dune in release mode fails with: >> ``` >> File "ocaml/xenstored/store.ml", line 464, characters 13-32: >> Warning 18: this type-based record disambiguation is not principal. >> File "ocaml/xenstored/store.ml", line 1: >> Error: Some fatal warnings were triggered (1 occurrences) >> ``` >> >> This is a warning to help keep the code futureproof, quoting from its >> documentation: >>> Check information path during type-checking, to make sure that all types >> are >>> derived in a principal way. When using labelled arguments and/or >> polymorphic >>> methods, this flag is required to ensure future versions of the compiler will >>> be able to infer types correctly, even if internal algorithms change. All >>> programs accepted in -principal mode are also accepted in the default >> mode with >>> equivalent types, but different binary signatures, and this may slow down >> type >>> checking; yet it is a good idea to use it once before publishing source code. >> >> Fixes: db471408edd46 "tools/ocaml/xenstored: Fix quota bypass on domain >> shutdown" > > Nit: The format of this "Fixes:" tag might need to be fixed? > >> >> Signed-off-by: Edwin Török <edvin.torok@citrix.com> >> --- >> Reason for inclusion in 4.17: >> - fixes a build error in a previous commit that is already in master > > Yes, given this is a simple enough patch: > > Release-acked-by: Henry Wang <Henry.Wang@arm.com> Afaics this patch was previously posted in isolation, and it was already release-acked. What's lacking there is a 2nd maintainer's ack or a proper R-b. When it now is patch 9 in a series, it isn't really obvious whether this could also be committed in isolation (it looks like it does, but a clear statement to this effect would have been beneficial). Jan
> On 9 Nov 2022, at 07:10, Jan Beulich <jbeulich@suse.com> wrote: > > On 09.11.2022 03:47, Henry Wang wrote: >>> -----Original Message----- >>> From: Edwin Török <edvin.torok@citrix.com> >>> Subject: [PATCH for-4.17 v3 09/15] tools/ocaml/xenstored/store.ml: fix build >>> error >>> >>> Building with Dune in release mode fails with: >>> ``` >>> File "ocaml/xenstored/store.ml", line 464, characters 13-32: >>> Warning 18: this type-based record disambiguation is not principal. >>> File "ocaml/xenstored/store.ml", line 1: >>> Error: Some fatal warnings were triggered (1 occurrences) >>> ``` >>> >>> This is a warning to help keep the code futureproof, quoting from its >>> documentation: >>>> Check information path during type-checking, to make sure that all types >>> are >>>> derived in a principal way. When using labelled arguments and/or >>> polymorphic >>>> methods, this flag is required to ensure future versions of the compiler will >>>> be able to infer types correctly, even if internal algorithms change. All >>>> programs accepted in -principal mode are also accepted in the default >>> mode with >>>> equivalent types, but different binary signatures, and this may slow down >>> type >>>> checking; yet it is a good idea to use it once before publishing source code. >>> >>> Fixes: db471408edd46 "tools/ocaml/xenstored: Fix quota bypass on domain >>> shutdown" >> >> Nit: The format of this "Fixes:" tag might need to be fixed? >> >>> >>> Signed-off-by: Edwin Török <edvin.torok@citrix.com> >>> --- >>> Reason for inclusion in 4.17: >>> - fixes a build error in a previous commit that is already in master >> >> Yes, given this is a simple enough patch: >> >> Release-acked-by: Henry Wang <Henry.Wang@arm.com> > > Afaics this patch was previously posted in isolation, and it was > already release-acked. What's lacking there is a 2nd maintainer's > ack or a proper R-b. When it now is patch 9 in a series, it isn't > really obvious whether this could also be committed in isolation > (it looks like it does, but a clear statement to this effect > would have been beneficial). > You're right it already has both acks, it just hasn't been commited yet: https://patchwork.kernel.org/project/xen-devel/patch/5a453393dad1de8286fe5db16504d3db2906eef8.1667500970.git.edvin.torok@citrix.com/ I've added the acks now to my github branch, so next time I resend the series it should be there. It can be applied independently, I've rebased and moved it at the beginning of the series (there was just some whitespace to fix up) If it helps here is the commit in isolation that could be cherry-picked onto master: https://github.com/edwintorok/xen/commit/da88b438e03da36212d07d24d67ab151ae287f4e Best regards, --Edwin
On 09.11.2022 10:21, Edwin Torok wrote: > > >> On 9 Nov 2022, at 07:10, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 09.11.2022 03:47, Henry Wang wrote: >>>> -----Original Message----- >>>> From: Edwin Török <edvin.torok@citrix.com> >>>> Subject: [PATCH for-4.17 v3 09/15] tools/ocaml/xenstored/store.ml: fix build >>>> error >>>> >>>> Building with Dune in release mode fails with: >>>> ``` >>>> File "ocaml/xenstored/store.ml", line 464, characters 13-32: >>>> Warning 18: this type-based record disambiguation is not principal. >>>> File "ocaml/xenstored/store.ml", line 1: >>>> Error: Some fatal warnings were triggered (1 occurrences) >>>> ``` >>>> >>>> This is a warning to help keep the code futureproof, quoting from its >>>> documentation: >>>>> Check information path during type-checking, to make sure that all types >>>> are >>>>> derived in a principal way. When using labelled arguments and/or >>>> polymorphic >>>>> methods, this flag is required to ensure future versions of the compiler will >>>>> be able to infer types correctly, even if internal algorithms change. All >>>>> programs accepted in -principal mode are also accepted in the default >>>> mode with >>>>> equivalent types, but different binary signatures, and this may slow down >>>> type >>>>> checking; yet it is a good idea to use it once before publishing source code. >>>> >>>> Fixes: db471408edd46 "tools/ocaml/xenstored: Fix quota bypass on domain >>>> shutdown" >>> >>> Nit: The format of this "Fixes:" tag might need to be fixed? >>> >>>> >>>> Signed-off-by: Edwin Török <edvin.torok@citrix.com> >>>> --- >>>> Reason for inclusion in 4.17: >>>> - fixes a build error in a previous commit that is already in master >>> >>> Yes, given this is a simple enough patch: >>> >>> Release-acked-by: Henry Wang <Henry.Wang@arm.com> >> >> Afaics this patch was previously posted in isolation, and it was >> already release-acked. What's lacking there is a 2nd maintainer's >> ack or a proper R-b. When it now is patch 9 in a series, it isn't >> really obvious whether this could also be committed in isolation >> (it looks like it does, but a clear statement to this effect >> would have been beneficial). >> > > > You're right it already has both acks, it just hasn't been commited yet: Oh, that's only because I overlooked Christian's ack. Will commit this now. Jan
On 09.11.2022 10:36, Jan Beulich wrote: > On 09.11.2022 10:21, Edwin Torok wrote: >> >> >>> On 9 Nov 2022, at 07:10, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 09.11.2022 03:47, Henry Wang wrote: >>>>> -----Original Message----- >>>>> From: Edwin Török <edvin.torok@citrix.com> >>>>> Subject: [PATCH for-4.17 v3 09/15] tools/ocaml/xenstored/store.ml: fix build >>>>> error >>>>> >>>>> Building with Dune in release mode fails with: >>>>> ``` >>>>> File "ocaml/xenstored/store.ml", line 464, characters 13-32: >>>>> Warning 18: this type-based record disambiguation is not principal. >>>>> File "ocaml/xenstored/store.ml", line 1: >>>>> Error: Some fatal warnings were triggered (1 occurrences) >>>>> ``` >>>>> >>>>> This is a warning to help keep the code futureproof, quoting from its >>>>> documentation: >>>>>> Check information path during type-checking, to make sure that all types >>>>> are >>>>>> derived in a principal way. When using labelled arguments and/or >>>>> polymorphic >>>>>> methods, this flag is required to ensure future versions of the compiler will >>>>>> be able to infer types correctly, even if internal algorithms change. All >>>>>> programs accepted in -principal mode are also accepted in the default >>>>> mode with >>>>>> equivalent types, but different binary signatures, and this may slow down >>>>> type >>>>>> checking; yet it is a good idea to use it once before publishing source code. >>>>> >>>>> Fixes: db471408edd46 "tools/ocaml/xenstored: Fix quota bypass on domain >>>>> shutdown" >>>> >>>> Nit: The format of this "Fixes:" tag might need to be fixed? >>>> >>>>> >>>>> Signed-off-by: Edwin Török <edvin.torok@citrix.com> >>>>> --- >>>>> Reason for inclusion in 4.17: >>>>> - fixes a build error in a previous commit that is already in master >>>> >>>> Yes, given this is a simple enough patch: >>>> >>>> Release-acked-by: Henry Wang <Henry.Wang@arm.com> >>> >>> Afaics this patch was previously posted in isolation, and it was >>> already release-acked. What's lacking there is a 2nd maintainer's >>> ack or a proper R-b. When it now is patch 9 in a series, it isn't >>> really obvious whether this could also be committed in isolation >>> (it looks like it does, but a clear statement to this effect >>> would have been beneficial). >>> >> >> >> You're right it already has both acks, it just hasn't been commited yet: > > Oh, that's only because I overlooked Christian's ack. Will commit this now. But, sigh, I had to fix up the patch: Even the one submitted standalone used space indentation when the file in the tree uses hard tabs. And even if I had wanted to pull from your github tree I would have had to fix up at least the Fixes: tag. So I ended up hand-editing indentation ... Jan
> On 9 Nov 2022, at 09:52, Jan Beulich <jbeulich@suse.com> wrote: > > On 09.11.2022 10:36, Jan Beulich wrote: >> On 09.11.2022 10:21, Edwin Torok wrote: >>> >>> >>>> On 9 Nov 2022, at 07:10, Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 09.11.2022 03:47, Henry Wang wrote: >>>>>> -----Original Message----- >>>>>> From: Edwin Török <edvin.torok@citrix.com> >>>>>> Subject: [PATCH for-4.17 v3 09/15] tools/ocaml/xenstored/store.ml: fix build >>>>>> error >>>>>> >>>>>> Building with Dune in release mode fails with: >>>>>> ``` >>>>>> File "ocaml/xenstored/store.ml", line 464, characters 13-32: >>>>>> Warning 18: this type-based record disambiguation is not principal. >>>>>> File "ocaml/xenstored/store.ml", line 1: >>>>>> Error: Some fatal warnings were triggered (1 occurrences) >>>>>> ``` >>>>>> >>>>>> This is a warning to help keep the code futureproof, quoting from its >>>>>> documentation: >>>>>>> Check information path during type-checking, to make sure that all types >>>>>> are >>>>>>> derived in a principal way. When using labelled arguments and/or >>>>>> polymorphic >>>>>>> methods, this flag is required to ensure future versions of the compiler will >>>>>>> be able to infer types correctly, even if internal algorithms change. All >>>>>>> programs accepted in -principal mode are also accepted in the default >>>>>> mode with >>>>>>> equivalent types, but different binary signatures, and this may slow down >>>>>> type >>>>>>> checking; yet it is a good idea to use it once before publishing source code. >>>>>> >>>>>> Fixes: db471408edd46 "tools/ocaml/xenstored: Fix quota bypass on domain >>>>>> shutdown" >>>>> >>>>> Nit: The format of this "Fixes:" tag might need to be fixed? >>>>> >>>>>> >>>>>> Signed-off-by: Edwin Török <edvin.torok@citrix.com> >>>>>> --- >>>>>> Reason for inclusion in 4.17: >>>>>> - fixes a build error in a previous commit that is already in master >>>>> >>>>> Yes, given this is a simple enough patch: >>>>> >>>>> Release-acked-by: Henry Wang <Henry.Wang@arm.com> >>>> >>>> Afaics this patch was previously posted in isolation, and it was >>>> already release-acked. What's lacking there is a 2nd maintainer's >>>> ack or a proper R-b. When it now is patch 9 in a series, it isn't >>>> really obvious whether this could also be committed in isolation >>>> (it looks like it does, but a clear statement to this effect >>>> would have been beneficial). >>>> >>> >>> >>> You're right it already has both acks, it just hasn't been commited yet: >> >> Oh, that's only because I overlooked Christian's ack. Will commit this now. > > But, sigh, I had to fix up the patch: Even the one submitted standalone > used space indentation when the file in the tree uses hard tabs. And > even if I had wanted to pull from your github tree I would have had to > fix up at least the Fixes: tag. I thought I fixed it (the missing '('), but the format of the Fixes: line was not documented in https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches (which is the canonical resource I use when sending patches) and I just tried to guess the format based on the Fixes: entries I found in master (some of which have or less characters in the git hash) I see there is a https://xenbits.xen.org/docs/unstable/process/sending-patches.html which has some more useful details, including a way to automatically generate the Fixes: line using a git config, which is very useful and I'll use that in the future instead of crafting it by hand (which is what I've been doing so far). I've edited the wiki to include this information now. > So I ended up hand-editing indentation Thanks, --Edwin
On 8 Nov 2022, at 15:34, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote: Building with Dune in release mode fails with: ``` File "ocaml/xenstored/store.ml", line 464, characters 13-32: Warning 18: this type-based record disambiguation is not principal. File "ocaml/xenstored/store.ml", line 1: Error: Some fatal warnings were triggered (1 occurrences) ``` This is a warning to help keep the code futureproof, quoting from its documentation: Check information path during type-checking, to make sure that all types are derived in a principal way. When using labelled arguments and/or polymorphic methods, this flag is required to ensure future versions of the compiler will be able to infer types correctly, even if internal algorithms change. All programs accepted in -principal mode are also accepted in the default mode with equivalent types, but different binary signatures, and this may slow down type checking; yet it is a good idea to use it once before publishing source code. Fixes: db471408edd46 "tools/ocaml/xenstored: Fix quota bypass on domain shutdown" Signed-off-by: Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> --- Reason for inclusion in 4.17: - fixes a build error in a previous commit that is already in master Changes since v2: - new in v3 --- tools/ocaml/xenstored/store.ml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>
diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml index 14ec404988..38a4945372 100644 --- a/tools/ocaml/xenstored/store.ml +++ b/tools/ocaml/xenstored/store.ml @@ -461,7 +461,7 @@ let reset_permissions store domid = | Some perms -> if perms <> node.perms then Logging.debug "store|node" "Changed permissions for node %s" (Node.get_name node); - Some { node with perms } + Some { node with Node.perms } ) store.root type ops = {