[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.19 at 14:34, <vliaskovitis@xxxxxxxx> wrote: > On Wed, 2019-03-13 at 17:35 +0100, Jan Beulich wrote: >> > > > On 06.03.19 at 13:58, <vliaskovitis@xxxxxxxx> wrote: >> > + found = false; >> >> I don't think you need this variable here, or if so, it shouldn't >> be boolean: Either you mean to support returning data for >> multiple matching options (there are no runtime ones at >> present, but we at least used to have multiple-instance >> boot time ones), in which case you may need to invent a >> means to disambiguate them. Or you don't, in which case >> you could bail from the loop once you've found a match. > > Multiple matching options was not my intention. Yes, the loop going > through the parameters should be exited on a match, but I still need to > detect if there was no parameter matched at all for the current option, > in order to return -EINVAL in that case. This can be detected from the > return value of strcmp, instead of using a boolean variable. From the return value of strcmp()? I don't think so, because you may have run past all table entries. Instead it's that property that you can use, i.e. checking whether ... >> > + for ( param = start; param < end; param++ ) ... the loop end condition was hit. >> > + { >> > + >> > + if ( strcmp(param->name, optkey) ) >> > + continue; >> > + >> > + found = true; >> > + switch ( param->type ) >> > + { >> > + case OPT_STR: >> > + len = snprintf((char*)val, sizeof(values), "%s ", >> >> Here and below, sizeof(values) is wrong in two ways: For one >> it's sizeof(char*), not sizeof(char[nn]). And then you need to >> subtract the amount of space consumed already. > > thanks, a basic mistake. I think the caller will need to pass the size > of the values[] buffer as an argument to the function, or I can always > assume a XEN_PARAMETER_SIZE max size for the values[] buffer. The > former sounds better to me. Indeed. >> > + (char*)param->par.var); >> > + val += len; >> > + break; >> > + case OPT_UINT: >> > + get_integer_param(param, ¶m_int); >> > + len = snprintf((char*)val, sizeof(values), "%lu ", >> > param_int); >> >> This is going to fail to build on 32-bit Arm. > > I will change the formatting to %"PRIu64" > > >> 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. >> > + param_int ? "true" : "false"); >> > + val += len; >> > + break; >> > + case OPT_SIZE: >> > + case OPT_CUSTOM: >> > + rc = -EINVAL; >> > + break; >> >> I can see why custom parameters can't be dealt with yet, >> but why also size ones? As to custom ones - four out of the >> seven runtime parameters we currently have are custom >> ones, so this limits the utility of the change quite a bit. > > From what I understand OPT_SIZE can be handled similarly to OPT_UINT, > printing an unsigned integer in the output. Right. > For OPT_CUSTOM, what about adding a get_func function pointer to their > definition in kernel_param, which would be used for reading the current > value? So the union par of struct kernel_param would now have a struct > with 2 function pointers for the custom parameters, something like: > > union { > void *var; > struct { > int (*func)(const char *); > const char *(*get_func)(void); > } custom; > } par; > > > 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? > 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? > The first option looks simpler to me. Simpler is often, but not always better. >> > @@ -199,6 +303,11 @@ int runtime_parse(const char *line) >> > return parse_params(line, __param_start, __param_end); >> > } >> > >> > +int runtime_get_parameter(const char *line, char *values) >> > +{ >> > + return get_params(line, values, __param_start, __param_end); >> > +} >> >> I don't see the need for this wrapper. > > I think I had trouble accessing __param_start, __param_end directly > from do_sysctl in xen/common/sysctl.c , that's why this was needed. But > perhaps I missed something obvious. You certainly shouldn't access them from sysctl.c. You should simply make get_params() access them instead of what its only caller passes, at which point the function should be renamed to it only caller's name. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |