Add 'disk signature' argument to 'make'?

[djs55]

We want to encourage users of this library to specify a disk signature, which could be used by an OS (like Mirage) to identify the disk.

[Burnleydev1]

Hello @reynir, I would like to work on this issue, please can you assign it to me. Thanks.

[Burnleydev1]

Hello @reynir please I wish to ask for guidance and pointers on how to fix this

[reynir]

Hello @Burnleydev1. I suggest looking at the wikipedia page on master boot record. It is the "modern standard MBR" we use. It is the make function you will need to modify:

ocaml-mbr/lib/mbr.ml

Line 165 in 9c8707b

let make partitions =

When working on the code you can use dune build to build and compile the project, and dune runtest to run the tests. It would be good to add tests, but feel free to create a draft pull request so we can give feedback.

[Burnleydev1]

Hello @reynir, please from this code you pointed to, there is a disk signature argument at line 209 already, so I wish to ask if maybe the fix existed already.

[reynir]

Yes, it's already there. But if you look a few lines up:

ocaml-mbr/lib/mbr.ml

Line 200 in 9c8707b

let disk_signature = 0l in

the disk_signature is a constant zero (0l as it is an int32). What we would like is to allow you to pass in a different value.

[Burnleydev1]

Hello @reynir Does this mean I can pass any unique value as long as it is int32?
For instance let disk_signature = 0xA1B2C3D4l

[reynir]

Yes, a user of make should be able to pass any int32 value, for example 0xA1B2C3D4l. The disk signature should be an argument to the function

[Burnleydev1]

Hello @reynir, I'm sorry for asking too many questions, but will we have to check for the correctness of the int32?

[reynir]

What would you check for? I think every int32 value is valid

[Burnleydev1]

This means will replace the line let disk_signature = 0l in to let disk_signature = read_line () in
and direct the user to input an int32 value.

[reynir]

The value for disk_signature is expected to be passed to the function as a function parameter, and not read and parsed from stdin. The function make already takes as argument partitions.

ocaml-mbr/lib/mbr.ml

Lines 165 to 210 in 9c8707b

	let make partitions =
	(if List.length partitions <= 4 then Ok () else Error "Too many partitions")
	>>= fun () ->
	let num_active =
	List.fold_left
	(fun acc p -> if p.Partition.active then succ acc else acc)
	0 partitions
	in
	(if num_active <= 1 then Ok ()
	else Error "More than one active/boot partitions is not advisable")
	>>= fun () ->
	let partitions =
	List.sort
	(fun p1 p2 ->
	Int32.unsigned_compare p1.Partition.first_absolute_sector_lba
	p2.Partition.first_absolute_sector_lba)
	partitions
	in
	(* Check for overlapping partitions *)
	List.fold_left
	(fun r p ->
	r >>= fun offset ->
	if
	Int32.unsigned_compare offset p.Partition.first_absolute_sector_lba <= 0
	then
	Ok (Int32.add p.Partition.first_absolute_sector_lba p.Partition.sectors)
	else Error "Partitions overlap")
	(Ok 1l) (* We start at 1 so the partitions don't overlap with the MBR *)
	partitions
	>>= fun (_ : int32) ->
	let bootstrap_code = String.init (218 + 216) (Fun.const '\000') in
	let original_physical_drive = 0 in
	let seconds = 0 in
	let minutes = 0 in
	let hours = 0 in
	let disk_signature = 0l in
	Ok
	{
	bootstrap_code;
	original_physical_drive;
	seconds;
	minutes;
	hours;
	disk_signature;
	partitions;
	}

[Burnleydev1]

Hello @reynir, when I ran dune build I got the error

File "lib/dune", line 6, characters 12-19:
6 |  (libraries cstruct)
               ^^^^^^^
Error: Library "cstruct" not found.
-> required by library "mbr-format" in _build/default/lib
-> required by _build/default/META.mbr-format
-> required by alias all
-> required by alias default
File "test/dune", line 3, characters 16-24:
3 |  (libraries mbr alcotest fmt))
                   ^^^^^^^^
Error: Library "alcotest" not found.
-> required by _build/default/test/test_mbr.exe
-> required by alias test/all
-> required by alias default
File "lib/dune", line 5, characters 7-18:     
5 |   (pps ppx_cstruct))
          ^^^^^^^^^^^
Error: Library "ppx_cstruct" not found.
-> required by _build/default/lib/mbr.pp.ml
-> required by alias lib/all
-> required by alias default

[reynir]

To install the required dependencies you can run opam install --deps-only .

[Burnleydev1]

Thank you @reynir, I did that and it worked,
I tried adding the disk_signature to the make function like this

let make partitions disk_signature =
  (if List.length partitions <= 4 then Ok () else Error "Too many partitions")....

but I noticed that the function could only take one parameter
please I am currently researching the correct way of doing this.
please bare with me as I am still very new to ocaml and how it works.

[PizieDust]

Thank you @reynir, I did that and it worked, I tried adding the disk_signature to the make function like this

let make partitions disk_signature =
  (if List.length partitions <= 4 then Ok () else Error "Too many partitions")....

but I noticed that the function could only take one parameter please I am currently researching the correct way of doing this. please bare with me as I am still very new to ocaml and how it works.

Hello @Burnleydev1
I think you also have to update the make function in the interface file mbr.mli so it recognizes the added parameter.

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

where int32 represents the type for disk_signature.
I'm a beginner too so maybe there could be some errors but I think when we update the function we have to update the interface file. Let me know if it works.

[Burnleydev1]

Hello @PizieDust, thanks for helping, i added this to the mb.mli and got a new error

58 |   | Ok _ -> ()
         ^^^^
Error: This pattern should not be a constructor, the expected type is
       int32 -> (Mbr.t, string) result

this error was showing in the test_mbr.ml file.

[Burnleydev1]

It worked and it seems that the test file will have to be updated to work with this new change.

[AryanGodara]

@reynir ,Can I give this a try, since I did something similar for #20 (reconsider ty argument)
It'll be quicker to implement this one too

[reynir]

@reynir ,Can I give this a try, since I did something similar for #20 (reconsider ty argument) It'll be quicker to implement this one too

Burnleydev1 has pretty much got it now. Do you want to work on this issue? reynir/mirage-block-partition#6

[AryanGodara]

@reynir ,Can I give this a try, since I did something similar for #20 (reconsider ty argument) It'll be quicker to implement this one too

Burnleydev1 has pretty much got it now. Do you want to work on this issue? reynir/mirage-block-partition#6

Yes, I'll work on this!
Will get started right now

Any pointers on where to begin @reynir ?