Add 'disk signature' argument to 'make' #19

Burnleydev1 · 2023-03-13T20:51:02Z

fixes #1

reynir · 2023-03-14T06:21:17Z

Great! As you note in #1 the tests would need to be updated. Another option would be to make disk_signature an optional argument https://v2.ocaml.org/releases/5.0/htmlman/lablexamples.html. This way users of the library (most likely) don't have to change their code.

Burnleydev1 · 2023-03-15T10:02:25Z

Thanks @reynir,
I’m currently trying to understand optional arguments from the link you sent.

Burnleydev1 · 2023-03-16T10:20:58Z

Hello @reynir, I have not changed the test_mbr.ml file, but I get errors like This pattern should not be a constructor, the expected type is int32 -> (Mbr.t, string) result
is given by the line | Ok _ -> () on line 58.

reynir

This is getting close!

reynir · 2023-03-16T12:41:25Z

lib/mbr.mli

@@ -79,11 +79,11 @@ type t = private {
  seconds : int;
  minutes : int;
  hours : int;
-  disk_signature : int32;
+  disk_signature : int32 option;


This does not need changing. We want the disk signature to always be present, and in the "smart" constructor (function make) we fill in a default value if not provided.

Suggested change

disk_signature : int32 option;

disk_signature : int32;

Will the default value be something like 0l?

something like let make (?disk_signature = 0l) in partitions

And change back the line disk_signature = (match disk_signature with Some s -> s | None -> 0l); to just disk_signature;

I updated the code to reflect the changes, but i wish to say the implementation of the optional argument is what I don't understand since the way I saw it used in the documentation you sent is using the ?disk_signature format. thanks

Yes, optional arguments in OCaml is a bit convoluted. There are many ways you can define a function with optional arguments: let f ?x () = Option.get x, let f ?(x=42) () = x, let f ?x:(y=42) () = y. Then there's a different syntax for calling the function, and for the type of the function. It may not have been the best very first issue, but I am glad that you have solved it!

reynir · 2023-03-16T12:42:59Z

lib/mbr.mli

  partitions : Partition.t list;
 }

-val make : Partition.t list -> (t, string) result
+val make : Partition.t list -> int32 -> (t, string) result


The signature for functions with optional arguments look a bit different. This signature is for a function with a mandatory unlabelled argument.

See https://v2.ocaml.org/releases/5.0/htmlman/lablexamples.html#s:optional-arguments

Hello @reynir, I'm sorry, but I am confused about what you mean, does it mean that ?disk_signature is a mandatory unlabelled argument?

Okay I think I get it, you mean, I have to create a signature, I never knew what that was, after doing some research, I wish this will do val make : ?disk_signature:int32 -> 'a list -> 'a
`

reynir · 2023-03-16T12:44:31Z

lib/mbr.ml

+  let signature_msg = match disk_signature with
+    | Some signature -> Printf.sprintf "Disk signature is: %ld\n" signature
+    | None -> "No disk signature provided\n" in
+  print_endline signature_msg;
+


Debug printing is fine, but before merging we should remove this :-)

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

Co-authored-by: Reynir Björnsson <reynir@reynir.dk> Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

reynir · 2023-03-17T15:37:36Z

lib/mbr.mli

 val make : Partition.t list -> (t, string) result
+
+val make : ?disk_signature:int32 -> 'a list -> 'a


You got it right with the ?disk_signature:int32 part. If you can add it to the type signature of make above that would be great. The rest of the type function of the latter make is not quite right.

Hello @reynir, thanks for the review, I just added it now.

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

lib/mbr.mli

Co-authored-by: Reynir Björnsson <reynir@reynir.dk>

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

@Burnleydev1

CHANGES: * Add optional argument `?disk_signature` to `Mbr.make` (@Burnleydev1, review by @reynir, mirage/ocaml-mbr#19) * Make the partition type a required argument to `Mbr.Partition.make` and rename it `~partition_type` (@AryanGodara, review by @reynir, mirage/ocaml-mbr#20) * Add tools for inspecting and modifying MBR, and reading/writing data to partitions. The command line tools are not installed as part of the opam package. The tools are `bin/mbr_inspect.exe`, `bin/read_partition.exe`, `bin/resize_partition.exe` and `bin/write_partition.exe`. (@PizieDust, review by @reynir, mirage/ocaml-mbr#22, mirage/ocaml-mbr#23, mirage/ocaml-mbr#24, mirage/ocaml-mbr#26) * Remove dependency on `ppx_cstruct` (@reynir, mirage/ocaml-mbr#27)

Burnleydev1 marked this pull request as draft March 13, 2023 20:52

reynir requested changes Mar 16, 2023

View reviewed changes

Burnleydev1 and others added 5 commits March 16, 2023 17:02

Add 'disk signature' argument to 'make'?

d230cf2

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

made disk_signature an optional argument

980191a

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

Update lib/mbr.mli

1817561

Co-authored-by: Reynir Björnsson <reynir@reynir.dk> Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

did changes requested

c11ba96

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

minor change'

645a4fd

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

Burnleydev1 force-pushed the disksig branch from 82f25db to 645a4fd Compare March 16, 2023 16:04

Burnleydev1 marked this pull request as ready for review March 17, 2023 14:07

Burnleydev1 added 3 commits March 17, 2023 15:15

Added default value to disk_signature

5c3a91d

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

dune fmt

71fbb31

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

added signature for disk_signature

b71f43c

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

Burnleydev1 requested a review from reynir March 17, 2023 14:33

reynir requested changes Mar 17, 2023

View reviewed changes

Burnleydev1 added 2 commits March 17, 2023 17:48

modified signature for disk_signature

5d688a0

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

formated the code using dune fmt

2032b6f

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

Burnleydev1 requested a review from reynir March 17, 2023 16:55

reynir approved these changes Mar 17, 2023

View reviewed changes

lib/mbr.mli Outdated Show resolved Hide resolved

Burnleydev1 and others added 4 commits March 17, 2023 17:59

Added missing comment tag

f768a17

Co-authored-by: Reynir Björnsson <reynir@reynir.dk>

fixed fmt of code

faf8558

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

fixed fmt of code

21ff506

Signed-off-by: Burnleydev1 <abongwabonalais@gmail.com>

Minor doc nits

809424b

reynir merged commit 783ca87 into mirage:master Mar 20, 2023

Burnleydev1 deleted the disksig branch March 20, 2023 12:58

reynir mentioned this pull request Apr 19, 2023

[new release] mbr-format (2.0.0) ocaml/opam-repository#23689

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'disk signature' argument to 'make' #19

Add 'disk signature' argument to 'make' #19

Burnleydev1 commented Mar 13, 2023

reynir commented Mar 14, 2023

Burnleydev1 commented Mar 15, 2023

Burnleydev1 commented Mar 16, 2023

reynir left a comment

reynir Mar 16, 2023

Burnleydev1 Mar 17, 2023

Burnleydev1 Mar 17, 2023 •

edited

Loading

Burnleydev1 Mar 17, 2023

Burnleydev1 Mar 17, 2023 •

edited

Loading

reynir Mar 17, 2023

reynir Mar 16, 2023

Burnleydev1 Mar 16, 2023

This comment was marked as off-topic.

Burnleydev1 Mar 17, 2023 •

edited

Loading

reynir Mar 16, 2023

reynir Mar 17, 2023

Burnleydev1 Mar 17, 2023

		val make : Partition.t list -> (t, string) result

		val make : ?disk_signature:int32 -> 'a list -> 'a

Add 'disk signature' argument to 'make' #19

Add 'disk signature' argument to 'make' #19

Conversation

Burnleydev1 commented Mar 13, 2023

reynir commented Mar 14, 2023

Burnleydev1 commented Mar 15, 2023

Burnleydev1 commented Mar 16, 2023

reynir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Burnleydev1 Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Burnleydev1 Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Burnleydev1 Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Burnleydev1 Mar 17, 2023 •

edited

Loading

Burnleydev1 Mar 17, 2023 •

edited

Loading

Burnleydev1 Mar 17, 2023 •

edited

Loading