On Mon, 2011-11-21 at 03:32 +0000, Rusty Russell wrote: > No it's not; I didn't bother when I converted things across, and it > shows. 546970bc (Rusty Russell 2010-08-11 23:04:20 -0600 133) #define module_param_cb Now I understand... :-) > But we expect virtio hackers to be smarter than average :) Hope I'll stand up to the challenge ;-) > > there would be a clash. So I wanted to add a "first_id" parameter, but <...> > Well, tell them to get the cmdline order right, or allow an explicit > device id in the commandline. Yeah, I've came up with the same idea last night: <size>[KMG]@<base>:<irq>[:<id>] <id>, if specified, sets the id for the current device and a base for the next one. > Since I hope we're going to be coding together more often, I've written > this how I would have done it (based loosely on your version) to > compare. Thanks, your efforts are truly appreciated! > Main changes other than the obvious: > 1) Documentation in kernel-parameters.txt I was considering this previously but for some reason I thought kernel-parameters.txt wasn't a place for module parameters at all. Now I had another look and see I was wrong. > 2) Doesn't leak mem on error paths. Em, I don't think my code was leaking memory in any error case? (no offence meant or taken ;-) > 3) Handles error from platform_device_register_resndata(). As my code was ignoring wrong descriptions (all the continues) I ignored this result as well (the implementation complains on its own anyway), but I get your point. > 4) Uses shorter names for static functions/variables. Ah, I get the hint :-) I'm trying to keep the naming convention in static symbols as well, as it makes cscope/ctags/grep usage easier... I'll just use the abbreviated "vm_" prefixes then. > See what you think... Funnily enough when I proposed some string parser few years ago (totally different story) I was flamed for using strchr() instead of strsep() ;-) But ok, I don't mind getting back to basics. > +static int set_cmdline_device(const char *device, const struct kernel_param *kp) [...] > + delim = strchr(device, '@'); [...] > + *delim = '\0'; Ah. I forgot that strchr() takes const char * but returns "non-const" char *... Cheating, that's what it is ;-), but will work. Probably what we really want is something like kstrtoull_delim(device, 0, &val, "@\0") I'll have a look and may try to propose something of that sort, but that's another story. Maybe I should just use simple_strtol() for the time being? I'll post v3 tonight or tomorrow. Cheers! Paweł _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
