Unikernel name changes from string to Vmm_core.Name.Label.t

[PizieDust]

closes #160

[hannesm]

I pushed a commit for albatross.ml to stick to the vmm_core.name.label.t a bit further.

[hannesm]

I pushed some further commits, please check that they're ok. I'm still curious why this PR adds ~70 lines of code. I think the whole multipart - POST - handling should be improved so we don't end up in these deeply nested matches. Especially this create_or_upload_volume is really tough to read, as well as update_settings, and unikernel_prepare_update.

[hannesm]

But it may be that if we just switch to vif, we get all the things for free. So maybe we should not invest too much into refactoring that right now.

[PizieDust]

This looks okay for me. I will merge later today. Thanks for the updates @hannesm

[hannesm]

maybe later we should take the time to push these Configuration.name_of_str further up.

[PizieDust]

sounds great. We can do this in a future PR for a general refactoring

[hannesm]

I'd appreciate more focussed PRs than "general refactoring".

[hannesm]

below here there's quite some refactoring that I don't fully grasp.

[PizieDust]

Here I just tried to reduced the amount of nesting at call sites.

previously the add_block function only performed the Block_add operation and the calling code was responsible for chaining the next step stream_to_albatross if the addition was successful.

and in addition we had a duplicated match to get the body of the post request (getting block name, size and compressed) which was duplicated for creating a block device and the same code for uploading to a block device.

With the refactoring, we get the post request body once, and then we check if we are creating, we just call add_block (which now internally calls stream_to_albatross) and if we are uploading to a block device, we directly call stream_to_albatross

[hannesm]

Why is this refactoring in this PR, and not in a separate one?

[PizieDust]

Given that albatross_state.query now needs a Vmm_core.Name.Label.t value for the ~name parameter, we have an issue with creating or uploading data to a block device, as we now have to pass the block name, which was previously a string, i.e, we now have to conver the string block_name to a Vmm_core.Name.Label.t without which this PR wouldn't compile.

without refactoring, this will become:

match parse_json with 
| ok json_body ->
    match create_or_upload, json_body with
    | create, block_params ->
         match config.name_of_str block_name with
         | err -> error
         | ok block name
                match token_or_cokie with 
                | token -> 
                      match add_block with 
                      | ok -> stream_to_albatross
                      | error -> return unit
                | cookie -> 
                      csrf_verification >>= 
                              match add_block with 
                              | ok -> stream_to_albatross
                              | error -> return unit
    | upload, block_params_without_block_size ->
          match config.name_of_str block_name with
          | err -> error
          | ok block name
                match token_or_cokie with 
                | token -> 
                      stream_to_albatross
                 | cookie -> 
                      csrf_verification >>= 
                           stream_to_albatross
| _ -> error

which is very messy with a lot of nested matching.

So currently with a little refactoring, we have:

match parse_json with
| block_params 
    match config.name_of_str block_name with
    | err -> error
    | ok block_name ->
           match token_or_cookie with 
           | token -> 
                 match create_or_upload with 
                 | create -> add_block
                 | upload -> stream_to_albatross
            | cookie ->
                  match create_or_upload with
                  | create -> csrf_verification >>= add_block
                  | upload -> csrf_verification >>= stream_to_albatross
| _ -> error

which is not still great but reduces a bit the messiness

[hannesm]

But additionally, the add_block and stream_to_albatross function definitions have been swapped, and in add_block, now stream_to_albatross is called. But I guess this is fine -- did you test this code esp. with the block_create and block_add operations?

[hannesm]

Thanks, merged. We can fix potential upcoming issues later.