|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 1/4] xen: add hypercall for getting parameters at runtime
On 18/03/2019 18:01, Jan Beulich wrote: >>>> On 18.03.19 at 16:44, <vliaskovitis@xxxxxxxx> wrote: >> On Mon, 2019-03-18 at 15:02 +0100, Jan Beulich wrote: >>>>> I'm also >>>>> unconvinced this is appropriate if the value is actually >>>>> a signed quantity. >>>> >>>> isn't OPT_UINT only for unsigned integers? >>> >>> You'll notice that there's no OPT_INT or OPT_SINT. OPT_UINT >>> may be slightly misleading as a name. Otoh we probably don't >>> have very many signed integer options. >> >> I see. Assuming an unsigned parameter is problematic then. >> >> Doesn't the .data.param section (between __param_start and __param_end) >> only include runtime parameters, of which the integer ones are all >> unsigned integers at the moment? Or would you like to see this >> functionality eventually used for all parameters, including non-runtime >> as well? > > Well, at the very least I expect restrictions to be called out very > clearly in the commit message. There not being any problematic > runtime parameters right now doesn't mean some might not > appear. And whether the problem would then be noticed is at > least unpredictable. IOW not dealing with the case right away > introduces a latent bug, and this shouldn't go unmentioned. > >>>> with the new get_func prototype being: >>>> >>>> const char *(*get_func)(void); >>>> >>>> returning a string with the current value of the parameter. >>> >>> And where would the storage live for the string? >> >> I was thinking these custom functions would return static strings, as >> loglvl_str() does, and these would eventually be copied into the user >> provided values[] buffer via snprintf in get_params(). > > Please don't limit your thinking to the few runtime parameters we > currently have. At least to me it is quite obvious that there can > easily be cases where static strings won't work. > >>>> e.g. the function for loglvl, guest_loglvl could return output >>>> using >>>> loglvl_str() : "Nothing", "Errors", "Errors and warnings" etc. >>>> >>>> An alternative prototype could be: >>>> >>>> int (*get_func)(char *output); >>>> >>>> if we want the function to write the current parameter value into a >>>> caller-provided buffer, and possibly return error codes. >>> >>> And how would the callee know how much space there is? >> >> yes, an extra size argument would be needed from the caller here. >> >> Let me know if either of these or a different approach is preferred. > > Of the two, I clearly prefer the latter. Then again me having pointed > out the shortcoming doesn't mean we (and hence you for this patch) > absolutely have to deal with custom parameters. Input from others > would certainly be helpful here. Why don't we replace the "*get_func()" with a "char *current_val" being filled at parameter setting time? How current_val is allocated (static string or dynamic buffer) just has to be known by the custom parameter parsing function. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |