Proposal: Add support for disks with custom block ops

[petrosagg]

Introduce a new function, lkl_disk_add2, that accepts one more
parameter than the original lkl_disk_add which allows the caller to use
a custom way of serving block requests.

lkl_disk_add is implemented as a trivial case for lkl_disk_add2 by
passing the default ops in order to not break existing users.

---

This change is 

[lkl-jenkins]

Can one of the admins verify this patch?

[tavip]

This is for adding support for other (then raw) disk image types, like qcow, right?

I think we should add the custom ops at the end of the lkl_disk structure, just like we do on the netdev side. That way we will not break the API. And in lkl_disk_add, if the ops field is null we can initialize it with the default ops.

Also, if you plan to add support for other disk image types, could you please take a look at how we did it for netdev and see if it fits?

[petrosagg]

@tavip Yes, this is to support things lke qcow images or encrypted images or (in my case) network backed images.

I changed my implementation to what you propose by adding a custom ops field at the end of the lkl_disk structure but during testing I found that it does change the API. Consider the following code which works with current LKL but breaks if we add an ops field at the end of lkl_disk.

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

#include <lkl.h>

int main()
{
    struct lkl_disk *bar;
    void *foo;

    foo = malloc(1024);
    memset(foo, '@', 1024);
    free(foo);

    bar = malloc(sizeof(struct lkl_disk));
    printf("bar->ops = %p\n", bar->ops);
}

Running this prints bar->ops = 0x4040404040404040 instead of (nil).

[tavip]

Sorry for the delay.

You are right :) The safe way would be to go with your proposal, but since we don't have too many users, I wonder if should break the API now in order to have a cleaner set of APIs in the future?

@Rondom @M1cha what do you think?

[M1cha]

@tavip the AUR says 'Required by (0)' and in UEFI everything is being linked statically anyway. So if you ask me you can break binary compatibility as often as you want :)

[Rondom]

If there is a valid reason (as in this case), I think one can break the API as necessary. LKL does not publish releases nor is it widespread enough yet that one needs to be overly careful.

[petrosagg]

@tavip Sounds good. I changed the PR to your suggested implementation

[tavip]

@lkl-jenkins: test this please

[tavip]

As you mentioned, this breaks the API, and we need to initialize the .ops field to NULL in the following apps: fs2tar.c, tests/boot.c and cptofs.c. lklfuse is fine, since there the lkl_disk is not allocated on stack but globally.

[petrosagg]

@tavip fixed

[tavip]

@lkl-jenkins: test this please