gaborigloi
commented
6 years ago These PRs have to be merged together:
Closed gaborigloi closed 6 years ago
gaborigloi
commented
6 years ago These PRs have to be merged together:
gaborigloi
commented
6 years ago This can be merged at any time, because currently it's not pinned to the target branch.
gaborigloi
commented
6 years ago We'll have to reset master to this branch, because they have diverged :open_mouth:
edwintorok
commented
6 years ago Reviewed 25 of 44 files at r1, 1 of 1 files at r2, 9 of 11 files at r3, 3 of 6 files at r4, 1 of 3 files at r5, 120 of 121 files at r6, 3 of 6 files at r7. Review status: 162 of 167 files reviewed, 8 unresolved discussions (waiting on @gaborigloi)
update_gh_pages.sh, line 34 at r7 (raw file):
git clone --quiet --branch=slate https://${GH_TOKEN}@github.com/xapi-project/xapi-storage $DOCDIR > /dev/null 2>&1 rm -rf $DOCDIR/source/includes/* _build/default/generator/src/main.exe gen_markdown --path=$DOCDIR/source/includes
BTW you can use jbuilder exec here and that'll keep working even if you use jbuild workspaces (the workspace usually puts things one directory level up).
generator/lib/common.ml, line 20 at r7 (raw file):
let task_id = Param.mk ~name:"task_id" Types.string type uri = string [@@deriving rpcty] [@@doc
BTWS is it worth to keep using @@doc or can we just use a regular OCaml comment, will the PPX consider both to be the same?
Might be easier to write and format comments if you don't need to quote each line.
generator/lib/control.ml, line 10 at r7 (raw file):
| Volume_does_not_exist of string (** The specified volume could not be found in the SR *) | Unimplemented of string (** The operation has not been implemented *) | Cancelled of string (** The operation has not been implemented *)
Docstring is wrong, it should say the 'operation has been canceled'
generator/lib/control.ml, line 289 at r7 (raw file):
let length = Param.mk ~name:"length" ~description: ["The length of the extent for which changed blocks should be computed"] Types.int
BTW should we use int64 here too for consistency?
generator/lib/control.ml, line 295 at r7 (raw file):
"have changed between [volume1] and [volume2] as a base64-encoded"; "bitmap string"] (dbg @-> sr @-> key @-> key2 @-> offset @-> length @-> returning changed_blocks errors)
Why are the types different for volume1 and volume2, are they named parameters?
generator/lib/control.ml, line 307 at r7 (raw file):
type configuration = (string * string) list [@@deriving rpcty] (** Plugin-specific configuration which describes where and; how to locate the storage repository. This may include; the
drop the ;
generator/lib/control.ml, line 308 at r7 (raw file):
(** Plugin-specific configuration which describes where and; how to locate the storage repository. This may include; the physical block device name, a remote NFS server and; path
is ; needed here?
generator/lib/data.ml, line 27 at r7 (raw file):
path: string; (** Path to the system local block device. This is equivalent to the SMAPIv1 params. *) dummy: unit;
What is the dummy for?
Comments from Reviewable
gaborigloi
commented
6 years ago update_gh_pages.sh, line 34 at r7 (raw file):
BTW you can use `jbuilder exec` here and that'll keep working even if you use jbuild workspaces (the workspace usually puts things one directory level up).
Thanks, fixed!
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/common.ml, line 20 at r7 (raw file):
BTWS is it worth to keep using `@@doc` or can we just use a regular OCaml comment, will the PPX consider both to be the same? Might be easier to write and format comments if you don't need to quote each line.
Yes, we can use ocaml comments most of the time, but not always - I'm planning to investigate & convert these later.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 10 at r7 (raw file):
Docstring is wrong, it should say the 'operation has been canceled'
Well spotted! Fixed it.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 289 at r7 (raw file):
BTW should we use int64 here too for consistency?
I used int here because users are not supposed to list changed blocks for such a large extent length that would require an int64 - that would result in significant memory usage. (CA-290243)
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 295 at r7 (raw file):
Why are the types different for volume1 and volume2, are they named parameters?
Exactly, the params have to be named for the Python generation to work (they are put into json dictionaries), that's why we need two different params with two different names.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 307 at r7 (raw file):
drop the ;
Well spotted!
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 308 at r7 (raw file):
is ; needed here?
Well spotted!
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 27 at r7 (raw file):
What is the dummy for?
Good point, now we can drop this! There was a bug in rpclib which caused a fatal warning for structs with only one field.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 27 at r7 (raw file):
Good point, now we can drop this! There was a bug in rpclib which caused a fatal warning for structs with only one field.
See https://github.com/mirage/ocaml-rpc/pull/102
Comments from Reviewable
edwintorok
commented
6 years ago Review status: 161 of 167 files reviewed, all discussions resolved (waiting on @edwintorok)
generator/lib/control.ml, line 289 at r7 (raw file):
I used int here because users are not supposed to list changed blocks for such a large extent length that would require an int64 - that would result in significant memory usage. (https://issues.citrite.net/browse/CA-290243)
OK. In practice you only loose 1 bit on 64-bit architectures, so int should be fine, we do not run on 32-bit architectures anyway.
Comments from Reviewable
edwintorok
commented
6 years ago Reviewed 2 of 44 files at r1, 1 of 6 files at r7, 3 of 3 files at r8. Review status: all files reviewed, 23 unresolved discussions (waiting on @gaborigloi)
generator/lib/control.ml, line 27 at r8 (raw file):
[@@deriving rpcty] type sr = string [@@deriving rpcty]
Could we use type sr = private string (assuming ocaml-rpc copes with that). That should ensure that XAPI won't accidentally change or use this in any way as a string. Its only the compiler that will know that this is a string, but otherwise its an abstract type at ocaml level too.
generator/lib/control.ml, line 117 at r8 (raw file):
(** Amount of space currently used on the backing storage (in bytes) *) uri : string list;
Didn't we drop URIs in favour of device-config? I'm a bit confused why its still here.
generator/lib/control.ml, line 132 at r8 (raw file):
type changed_blocks = { granularity: int;
What units does granularity use, is it bytes?
generator/lib/control.ml, line 133 at r8 (raw file):
type changed_blocks = { granularity: int; bitmap: string;
Could we define a base64 abstract Rpc type and conversion functions to/from string? i.e. something like this but for base64:
let typ_of_ipaddr =
Rpc.Types.Abstract
{ aname= "ipaddr"
; test_data= [Ipaddr.of_string_exn "127.0.0.1"]
; rpc_of= (fun t -> Rpc.String (Ipaddr.to_string t))
; of_rpc=
(function
| Rpc.String s -> (
try Ok (Ipaddr.of_string_exn s)
with Ipaddr.Parse_error (msg, arg) -> R.error_msgf "%s: %s" msg arg
)
| r ->
R.error_msgf "typ_of_ipaddr: expected rpc string got %s"
(Rpc.to_string r)) } generator/lib/control.ml, line 133 at r8 (raw file):
type changed_blocks = { granularity: int; bitmap: string;
What is the meaning of the bitmap? bit is set if block has changed?
generator/lib/control.ml, line 134 at r8 (raw file):
changed_blocksWill this be a single struct, or a list of extents (list of changed_blocks chunks) like we have in sparse_dd? Even for a ~2TiB disk this would be ~90MiB considering 4KiB block sizes. If we want to support >2TiB (e.g. 1PiB) we would need several hundreds of gigabytes of memory, so would be better if this was streamed.
generator/lib/control.ml, line 283 at r8 (raw file):
changed_blocksIt is slightly confusing to have both parameter types and methods declared at topmost level. Would it make sense to nest the declaration of the offset type in the method that uses it, assuming its just one place?
generator/lib/control.ml, line 302 at r8 (raw file):
description=["Operations which operate on volumes (also known as "; "Virtual Disk Images)"]; version=(1,0,0)}
We've already released API version 4, so this should be version 5. We've got plenty of integers, lets not worry about keeping them low :)
generator/lib/control.ml, line 420 at r8 (raw file):
namespace = Some "SR"; description=["Operations which act on Storage Repositories"]; version=(1,0,0)}
Version 5 please, we already released version 4.
generator/lib/data.ml, line 179 at r8 (raw file):
simultaneously. |}]; version=(1,0,0);
Already released version 4, lets start from 5 here.
generator/lib/data.ml, line 217 at r8 (raw file):
blocklistWhere is blocklist defined?
generator/lib/data.ml, line 227 at r8 (raw file):
"`nbd://root:pass@foo.com/path/to/disk` that must contain all necessary"; "authentication tokens"] (dbg @-> uri_p @-> domain @-> remote @-> blocklist @-> returning operation error)
Will this use NBD over TLS? Would be good if we can ensure that this path is always encrypted.
generator/lib/data.ml, line 255 at r8 (raw file):
let ls = declare "ls" ["[ls] returns a list of all current operations"] (dbg @-> returning operations error)
I would've expected the Task interface to be used here, any reason why it isn't?
generator/lib/data.ml, line 283 at r8 (raw file):
`Data.mirror` API call. 4. Find the list of blocks to copy via the CBT API call. Note that if
Does this mean that all SMAPIv3 SRs must support CBT if they support SxM?
generator/lib/data.ml, line 297 at r8 (raw file):
7. Once the copy operation has succesfully completed the destination disk will be a perfect mirror of the source.
Would be good if there was another optional operation available here that took a snapshot and calculated a SHA256 (or some other) hash of the VDI, and then did the same on the destination after all (delta) copies have finished. We could use this during a quicktest, or just in general in some "debug" mode and run a stress test.
generator/lib/plugin.ml, line 4 at r8 (raw file):
open Idl open Common
What happend to the Unimplemented exception here? Or do we simply require all plugin methods to be implemented?
generator/lib/plugin.ml, line 28 at r8 (raw file):
["minimum required API version"]]; features : string list [@doc
Could we use an enum here, or the "wire" format would not be compatible?
generator/lib/plugin.ml, line 34 at r8 (raw file):
["key/description pairs describing required device_config parameters"]]; required_cluster_stack : string list [@doc
Same question here, would using an enum break the 'wire API'? Unfortunately Plugin is the one API we cannot break, because it is the one used to determine the version number of the plugin.
generator/lib/plugin.ml, line 80 at r8 (raw file):
"must support the query interface or it will not be recognised as"; "a storage plugin by xapi."]; version=(1,0,0)}
1.0 is correct here, we can't ever change this API, see above.
generator/lib/plugin.ml, line 87 at r8 (raw file):
let interfaces = Codegen.Interfaces.create ~name:"plugin" ~title:"The Datapath plugin interface"
This seems to have been taken from the wrong place. This is Plugin.ml, so it should say something like the old one did
Interfaces.name = "plugin";
title = "The Plugin interface";
description =
String.concat " " [
"The xapi toolstack expects all plugins to support a basic query";
"interface.";
]; generator/lib/task.ml, line 1 at r8 (raw file):
(* Tasks API *)
Is this the equivalent of the Task API from https://github.com/xapi-project/xcp-idl/blob/b062924ccc99713e9836e5c9b33b30734febce85/storage/storage_interface.ml#L168-L192
generator/lib/task.ml, line 14 at r8 (raw file):
type async_result_t = | UnitResult of unit | Volume of Control.volume
There is also a Mirror.id in the xcp-idl enum, is that needed here?
generator/lib/task.ml, line 33 at r8 (raw file):
debug_info : string; ctime : float; state : state;
subtasks, debug_info and backtrace are missing compared to xcp-idl.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 27 at r8 (raw file):
Could we use `type sr = private string` (assuming ocaml-rpc copes with that). That should ensure that XAPI won't accidentally change or use this in any way as a string. Its only the compiler that will know that this is a string, but otherwise its an abstract type at ocaml level too.
No, sadly that didn't work, it confused the ppx somehow :/
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 117 at r8 (raw file):
Didn't we drop URIs in favour of device-config? I'm a bit confused why its still here.
These URIs are different, they are for accessing the data of the Volume via a datapath plugin, not for identifying or locating it. We use them in xapi-storage-script to select a datapath based on the URI scheme, in choose_datapath. But maybe the documentation could be improved to clarify that, I'm not sure how
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 132 at r8 (raw file):
What units does granularity use, is it bytes?
Yes, I think that makes the most sense, I'll document that
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 133 at r8 (raw file):
Could we define a base64 abstract Rpc type and conversion functions to/from string? i.e. something like this but for base64: ``` let typ_of_ipaddr = Rpc.Types.Abstract { aname= "ipaddr" ; test_data= [Ipaddr.of_string_exn "127.0.0.1"] ; rpc_of= (fun t -> Rpc.String (Ipaddr.to_string t)) ; of_rpc= (function | Rpc.String s -> ( try Ok (Ipaddr.of_string_exn s) with Ipaddr.Parse_error (msg, arg) -> R.error_msgf "%s: %s" msg arg ) | r -> R.error_msgf "typ_of_ipaddr: expected rpc string got %s" (Rpc.to_string r)) } ```
I think we should leave it in base64 for now, because that is the format we return from the relevant XenAPI call, and in the current CBT implementation we pass it up from SM without conversion to/from base64.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 134 at r8 (raw file):
> ``` > changed_blocks > ``` Will this be a single struct, or a list of extents (list of changed_blocks chunks) like we have in sparse_dd? Even for a ~2TiB disk this would be ~90MiB considering 4KiB block sizes. If we want to support >2TiB (e.g. 1PiB) we would need several hundreds of gigabytes of memory, so would be better if this was streamed.
The API is now chunked, we have to request changed blocks for a sub-area of the disk, and pass in the offset, and the length. This is to fix the issue you mention about excessive memory consumption etc.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 133 at r8 (raw file):
What is the meaning of the bitmap? bit is set if block has changed?
Yes, I've updated the docs
Comments from Reviewable
edwintorok
commented
6 years ago Reviewed 1 of 1 files at r9. Review status: all files reviewed, 21 unresolved discussions (waiting on @gaborigloi and @edwintorok)
generator/lib/control.ml, line 117 at r8 (raw file):
These URIs are different, they are for accessing the data of the Volume via a datapath plugin, not for identifying or locating it. We use them in xapi-storage-script to select a datapath based on the URI scheme, in `choose_datapath`. But maybe the documentation could be improved to clarify that, I'm not sure how
Perhaps say that it can be used to 'by a datapath plugin for I/O' instead of just I/O
generator/lib/control.ml, line 121 at r8 (raw file):
reference a local block device, a remote NFS share, iSCSI LUN or RBD volume. In cases where the data may be accessed over several protocols, he list should be sorted into descending order of
typo: /he list/the list/
generator/lib/control.ml, line 133 at r8 (raw file):
I think we should leave it in base64 for now, because that is the format we return from the relevant XenAPI call, and in the current CBT implementation we pass it up from SM without conversion to/from base64.
can you define a type base64 = string [@@deriving ..., won't prevent mistakenly putting another string there but would make it clearer when looking at types that this is not a regular string.
generator/lib/control.ml, line 134 at r8 (raw file):
The API is now chunked, we have to request changed blocks for a sub-area of the disk, and pass in the offset, and the length. This is to fix the issue you mention about excessive memory consumption etc.
Thanks, would it help to include in the reply the offset and length, so the struct is self-contained and unambiguously describes a changed block chunk?
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 283 at r8 (raw file):
> ``` > changed_blocks > ``` It is slightly confusing to have both parameter types and methods declared at topmost level. Would it make sense to nest the declaration of the offset type in the method that uses it, assuming its just one place?
Good idea, I've done this for all affected params, I think for now they are not needed elsewhere.
Comments from Reviewable
edwintorok
commented
6 years ago Reviewed 1 of 1 files at r10. Review status: all files reviewed, 20 unresolved discussions (waiting on @gaborigloi)
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 302 at r8 (raw file):
We've already released API version 4, so this should be version 5. We've got plenty of integers, lets not worry about keeping them low :)
Yep, we have a ticket for fixing this, and making xapi-storage-script check the versions properly, I was planning to do this after the merge, to avoid adding even more change to this branch, because this requires more changes, like renaming directories etc.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 420 at r8 (raw file):
Version 5 please, we already released version 4.
Please see my above comment - I'm planning to do this after the merge because this is a larger change.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 179 at r8 (raw file):
Already released version 4, lets start from 5 here.
Same comment as above - I'm planning to sort out versioning after the merge as part of another ticket.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 217 at r8 (raw file):
> ``` > blocklist > ``` Where is blocklist defined?
IT comes from the Common module
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 227 at r8 (raw file):
Will this use NBD over TLS? Would be good if we can ensure that this path is always encrypted.
I think this is not implemented currently. But I agree that we should do encrypted NBD.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 255 at r8 (raw file):
I would've expected the Task interface to be used here, any reason why it isn't?
I'm not sure, but this may not be implemented either. These new parts of the interface are not that important, and we might change them in the future.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 283 at r8 (raw file):
Does this mean that all SMAPIv3 SRs must support CBT if they support SxM?
Not sure, according to the current plan we will leave the migration code in xapi unchanged.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 283 at r8 (raw file):
Not sure, according to the current plan we will leave the migration code in xapi unchanged.
But this has not been implemented yet, so it might also change in the future.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 297 at r8 (raw file):
Would be good if there was another optional operation available here that took a snapshot and calculated a SHA256 (or some other) hash of the VDI, and then did the same on the destination after all (delta) copies have finished. We could use this during a quicktest, or just in general in some "debug" mode and run a stress test.
THere is an existing xenapi call that does that, VDI.checksum - it attaches the VDI to dom0 and then checksums it. But I think these steps are subject to change, because it has not been implemented yet.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/plugin.ml, line 4 at r8 (raw file):
What happend to the Unimplemented exception here? Or do we simply require all plugin methods to be implemented?
Yes, I think that's important because these methods are essential.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/plugin.ml, line 28 at r8 (raw file):
Could we use an enum here, or the "wire" format would not be compatible?
I think it might be too restrictive to do that, plus the wire format needs to stay compatible with the old PVS scripts.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/plugin.ml, line 34 at r8 (raw file):
Same question here, would using an enum break the 'wire API'? Unfortunately Plugin is the one API we cannot break, because it is the one used to determine the version number of the plugin.
Same as above - I think an enum would be too restrictive. The wire API may not break if we include in the enums the possible strings that the PVS scripts return, but it's best not to risk it in my opinion.
Comments from Reviewable
edwintorok
commented
6 years ago Reviewed 3 of 3 files at r11. Review status: all files reviewed, 13 unresolved discussions (waiting on @gaborigloi and @edwintorok)
generator/lib/control.ml, line 302 at r8 (raw file):
Yep, we have a ticket for fixing this, and making xapi-storage-script check the versions properly, I was planning to do this after the merge, to avoid adding even more change to this branch, because this requires more changes, like renaming directories etc.
Even if we do not check the version yet, can we bump the number here? Would that change anything in directory layout, etc?
generator/lib/data.ml, line 227 at r8 (raw file):
I think this is not implemented currently. But I agree that we should do encrypted NBD.
Can you create a ticket for that (or update the SxM ticket)? IIUC these PRs just prepares the APIs to make SxM possible, but don't actually start using it from XAPI yet. So as long as we return a proper URI we can choose between TLS and non-TLS. E.g. we could say that nbd:// by default is TLS and you have to use nbd-insecure:// if you want the non-TLS version. This would match with the reasoning in xapi-nbd: make the TLS variant easy to use, and make it hard or inconvenient to use the non-TLS variant to encourage using the TLS variant wherever possible. If we make the TLS variant the default when we implement then it should be fine.
generator/lib/data.ml, line 255 at r8 (raw file):
I'm not sure, but this may not be implemented either. These new parts of the interface are not that important, and we might change them in the future.
Where is the Task interface going to be used? Would the SMGC show up as a task?
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/plugin.ml, line 87 at r8 (raw file):
This seems to have been taken from the wrong place. This is Plugin.ml, so it should say something like the old one did ``` Interfaces.name = "plugin"; title = "The Plugin interface"; description = String.concat " " [ "The xapi toolstack expects all plugins to support a basic query"; "interface."; ]; ```
Thanks, fixed!
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/task.ml, line 1 at r8 (raw file):
Is this the equivalent of the Task API from https://github.com/xapi-project/xcp-idl/blob/b062924ccc99713e9836e5c9b33b30734febce85/storage/storage_interface.ml#L168-L192
I'm not sure, because I have not written this, but I think it's still subject to change, and isn't yet implemented, so it's probably not worth going through it in detail.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/task.ml, line 14 at r8 (raw file):
There is also a Mirror.id in the xcp-idl enum, is that needed here?
See above comment - we might add it later when we start to implement this interface.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/task.ml, line 33 at r8 (raw file):
subtasks, debug_info and backtrace are missing compared to xcp-idl.
We might add these later when we start to implement it, but for now, I'm not sure what it should look like. When we get to implementing it, I'll find out whether or not it should be the same as xcp-idl.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/task.ml, line 33 at r8 (raw file):
We might add these later when we start to implement it, but for now, I'm not sure what it should look like. When we get to implementing it, I'll find out whether or not it should be the same as xcp-idl.
But for now it's just the best to ignore these unused new interfaces, because they won't cause any problems.
Comments from Reviewable
edwintorok
commented
6 years ago Reviewed 1 of 1 files at r12. Review status: all files reviewed, 9 unresolved discussions (waiting on @gaborigloi and @edwintorok)
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 117 at r8 (raw file):
Perhaps say that it can be used to 'by a datapath plugin for I/O' instead of just I/O
Done.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 121 at r8 (raw file):
typo: /he list/the list/
Done.
Comments from Reviewable
edwintorok
commented
6 years ago Reviewed 1 of 1 files at r13, 1 of 1 files at r14. Review status: all files reviewed, 7 unresolved discussions (waiting on @gaborigloi and @edwintorok)
generator/lib/control.ml, line 420 at r8 (raw file):
Even if we do not check the version yet, can we bump the number here? Would that change anything in directory layout, etc?
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 133 at r8 (raw file):
can you define a `type base64 = string [@@deriving ...`, won't prevent mistakenly putting another string there but would make it clearer when looking at types that this is not a regular string.
Done.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 134 at r8 (raw file):
Thanks, would it help to include in the reply the offset and length, so the struct is self-contained and unambiguously describes a changed block chunk?
We've discussed with Stefano a while ago that it's not important to do this, because the caller should know what it asked for.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/control.ml, line 302 at r8 (raw file):
Even if we do not check the version yet, can we bump the number here? Would that change anything in directory layout, etc?
I'd prefer to do that together with all the other related changes, e.g. changing the v4 directories to v5. And I'm not sure whether it would break anything.
Comments from Reviewable
gaborigloi
commented
6 years ago generator/lib/data.ml, line 227 at r8 (raw file):
Can you create a ticket for that (or update the SxM ticket)? IIUC these PRs just prepares the APIs to make SxM possible, but don't actually start using it from XAPI yet. So as long as we return a proper URI we can choose between TLS and non-TLS. E.g. we could say that nbd:// by default is TLS and you have to use nbd-insecure:// if you want the non-TLS version. This would match with the reasoning in xapi-nbd: make the TLS variant easy to use, and make it hard or inconvenient to use the non-TLS variant to encourage using the TLS variant wherever possible. If we make the TLS variant the default when we implement then it should be fine.
Hmm, I think the URI scheme is not finalized yet. A ticket for making sure we use encrypted NBD for SXM? I can do that. Encrypted NBD is not that important at the moment, because we have a local NBD server accessed via a unix domain socket.
Comments from Reviewable
I've moved the
feature/REQ477/masterbranch to the base commit where I started from, so that all my changes are visible here.This change is