Re: [Minios-devel] [UNIKRAFT PATCH 8/8] lib/uklibparam: Add documentation for parameters

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

On 4/10/19 3:21 PM, Florian Schmidt wrote:
> Hi Sharan,
>
> originally, I started to review your code from the beginning, but then I 
> figured, maybe I should start with reading your documentation and try to 
> use it without knowing all the ins and outs of the code yet. I have a 
> few remarks below that would clarify things, I think:
> On 3/15/19 6:06 PM, Sharan Santhanam wrote:
> > This patch adds documentation for Unikraft library argument
> > library.
> >
> > Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> > ---
> >   doc/guides/developers-app.rst | 72 
> > +++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 72 insertions(+)
> >
> > diff --git a/doc/guides/developers-app.rst 
> > b/doc/guides/developers-app.rst
> > index 2a67b1f..fe779a4 100644
> > --- a/doc/guides/developers-app.rst
> > +++ b/doc/guides/developers-app.rst
> > @@ -323,6 +323,78 @@ An example context of extra.ld: ::
> >   This will add the section .uk_fs_list after the .text
> > +==================================
> > +Command line arguments in Unikraft
> > +==================================
> > +Unikraft command line arguments could either belong to an application 
> > or to a
> > +library. These two classes of arguments are separated by **--**. The
> > +arguments meant to a library precedes the **--**, followed by the 
> > application
> > +arguments.
> > +
> > +Pass an argument to a library
> > +-----------------------------
> > +Unikraft provides support to pass arguments of the following data type:
> > +
> > +========  ========================
> > +Type      Description
> > +========  ========================
> > +char      Single character value
> I would point out here explicitly that "char" is an alias for "__s8". 
> It's pretty obvious in the case of char...
Sure I will.
> > +__s8      Same as char
> > +__u8      Single byte value
> > +__s16     Short signed integer
> > +__u16     Short unsigned integer
> > +int       Integer
> ... but here, it might not necessarily be obvious that "int" is an alias 
> for "__s32" in all situations.
I will update it.
> > +__s32     Signed integer
> > +__u32     Unsigned integer
> > +__s64     Signed long integer
> > +__u64     Unsigned long integer
> > +charp     C strings.
> > +========  ========================
> > +
> > +There are three interfaces through which a library registers with a 
> > variable
> > +with Unikraft. These are:
> > +
> > +* UK_LIB_PARAM     - Pass a scalar value of the above type to a 
> > variable.
> > +* UK_LIB_PARAM_STR - Pass a null terminated string to a variable.
> > +* UK_LIB_PARAM_ARR - Pass space separated list of values of the above 
> > type.
> This should maybe shortly explain how many arguments each macro takes 
> and, in one or two words each, their meanings.
I wouldn't add the parameter as they are part of the API documentation. 
I would instead add two more examples for the String and array.
> > +
> > +Each library argument is identified by the following format ::
> > +
> > + [library name].[variable name]
> > +
> > + where,
> > +     library name is the name registered with Unikraft build system.
> > +     variable name is the name of the global or static variable in 
> > the program.
> > +
> > +Examples
> > +--------
> > +If the library requires command line argument support, it needs some
> > +configuration to be performed while building the library. A Unikraft 
> > library can
> > +be specific to a particular platform or common across all the platform.
> > +For the common library, one has to edit the Makefile.uk with ::
> > +
> > + $(eval $(call addlib_paramprefix,libukalloc,alloc))
> > + where,
> > +      libukalloc is the name of the library
> > +      alloc is the alias for the library name.
> > +
> > +or for the platform library, edit the Makefile.uk with ::
> > +
> > + $(eval $(call addplatlib_paramprefix,linuxu,liblinuxuplat,linuxu))
> > + where,
> > +     linuxu is the Unikraft platform
> > +     liblinuxuplat is the name of the library
> > +     linuxu an alias for the library name.
> > +
> > +As the next step, we define a variable and register it with the 
> > ``uk_libparam``
> > +library. The example below a simple code snippet ::
> > +
> > + static __u32 heap_size = CONFIG_LINUXU_DEFAULT_HEAPMB;
> > + UK_LIB_PARAM(heap_size, __u32);
> I think this might be confusing. At first, I thought I might actually 
> need to define a CONFIG_* definition for every library parameter. So 
> maybe a simpler example of "static __u16 myparam = 23" as a first 
> example would be better, and then you can continue with "of course, we 
> can also initialize the parameter with a compile-time configuration 
> setting from menuconfig" or something like that.
> Also, you don't mention that uk/libparam.h needs to be included.
Agree
> > +
> > +We can override the default value using the following command line ::
> > +
> > +  ./unikraft_linuxu-x86_64 linuxu.heap_size=10 --
> >   ============================
> >   Make Targets
> Those are the remarks I have so far. If I come up with more, I'll add 
> comments later. :)
> Cheers,
> Florian
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel