Re: [Minios-devel] [UNIKRAFT PATCH 1/8] lib/uklibparam: Introduce the library parameter

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Sharan,

On 4/15/19 3:12 PM, Sharan Santhanam wrote:
> On 4/11/19 4:13 PM, Florian Schmidt wrote:
> > On 3/15/19 6:06 PM, Sharan Santhanam wrote:
> > > +struct param_args {
> > > +    /* Reference to the start of the library */
> > > +    char *lib;
> > > +    /* Reference to the start of the parameter */
> > > +    char *param;
> > > +    /* Reference to the start of the value */
> > > +    char *value;
> > > +    /* length of the library name */
> > > +    __u8 lib_len;
> > > +    /* length of the parameter */
> > > +    __u8 param_len;
> > > +    /* length of the value */
> > > +    __u8 value_len;
> > > +};
> > Is there a reason you make these variables a __u8? I understand that 
> > parameters longer than 255 characters would be unusual, but I don't 
> > see an advantage in limiting the values like that. Why not make them 
> > an unsigned int or something and make use of the memory that's gonna 
> > get used for the struct anyway (due do padding)?
> The reason for __u8 was 255 is sufficient. I could make them __u32.
Ah, but you can't know that it always be sufficient, right? I just 
figured, it makes no difference at all in the size of the struct if you 
use __u16 or even __u32, so you might as well... unless there are 
reasons you want this to not go higher than 255.

> > You also invest a lot of effort into parsing differently-spaced 
> > versions of arguments: "lib.param=value", "lib.param = value", 
> > "lib.param= value", etc... I would've just thrown a bit fat error to 
> > the user and told him to stop being stupid with spaces in arguments. ;-)
> I would suggest keeping it, as it increase usablity
Oh, yes, by all means, keep it, I was just saying I wouldn't have been 
so user friendly!

Cheers,
Florian

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel