<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 26, 2015 at 6:22 PM, Zbigniew Jędrzejewski-Szmek <<a href="mailto:zbyszek@in.waw.pl" target="_blank">zbyszek@in.waw.pl</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, Feb 26, 2015 at 08:04:08AM +0000, Jan Janssen wrote: > Shawn Landden <shawn <at> <a href="http://churchofgit.com" target="_blank">churchofgit.com</a>> writes: > > > void strv_free(char **l) { > > - strv_clear(l); > > + char **k; > > + > > + if (!l) > > + return; > > + > > + for (k = l; *k; k++) > > + free(*k); > > + > > free(l); > > } > What are you trying to achieve here? I see no point in optimizing out the *l > = NULL from strv_clear. > > > + entry->linux_loc = l + strspn(l, > WHITESPACE); > > + else if ((l = startswith(m, "initrd "))) > > + entry->initrd = l + strspn(l, > WHITESPACE); > You need to support more than one initrd per kernel, see > <a href="https://wiki.archlinux.org/index.php/Microcode" target="_blank">https://wiki.archlinux.org/index.php/Microcode</a> for why. Also, I am pretty > sure you can have a initrd=/path/to/initrd in the kernel options entry. > Since the efi bootloader just appends each given initrd to the kernel > command line. > > > All in all I am wondering why you need a rbtree for all this in the first > place? A simple hashmap should do just fine. A hashmap does not keep order. But a simple array + qsort_safe() should work too. I'm wary of introducing yet another data structure into systemd which raises the bar for people editing the code later on or making changes. We need two operations: sorting kernels to list them, and picking (I presume) the latest kernel or the kernel with the given version. Both of those operations are done once over the lifetime of the program, so any speedup in using a data structure should take into account the time to set up the structure. Neither of those operations is speed sensitive, and the more common operation of picking a specific version can be done in O(n) over an array. So using an rbtree will not save any time actually. </blockquote><div>I was initially using a vector of pointers here for the same reasons you reiterated, but I felt the use of greedy_realloc0() was messy and error-prone. The rbtree does not require the use of realloc(). There is no way to know how long the array needs to be from the start. Even the O(n) you mention could be turned into O(logn) by using a binary search.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > Also, you're not taking multi-boot into account (the machine-id field). > You're just discriminating based on the kernel version, but different > installations could have the same version field. This high-level design questions should probably be addressed first... Zbyszek _______________________________________________ systemd-devel mailing list <a href="mailto:systemd-devel@lists.freedesktop.org">systemd-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/systemd-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/systemd-devel</a> </blockquote></div> <div> </div>-- <div class="gmail_signature">Shawn Landden</div> </div></div>