Code review comment for lp:~jamesodhunt/ubuntu/precise/upstart/1.4

Revision history for this message
Colin Watson (cjwatson) wrote :

-syn keyword upstartStatement umask nice oom chroot chdir exec
+syn keyword upstartStatement umask nice oom chroot chdir exec setiud setgid

Typo: setiud -> setuid.

+but with the
+.B --debug
+option.

\-\-debug here (it's correct elsewhere).

                var = NIH_MUST (nih_sprintf (NULL, "%s=%s", key,
- udev_list_entry_get_value (list_entry)));
+ copy_string (NULL, udev_list_entry_get_value (list_entry))));

Isn't this a memory leak? Since copy_string is called with a NULL parent, it will (as I understand it) need to be freed either by calling nih_free explicitly or by assigning it to something that's nih_local. That doesn't seem to be done in this case. Admittedly, this is a bit less important in the udev bridge than in the init daemon proper.

+ /* If substitutions were necessary, shrink the string */
+ return i == j ? cleaned : nih_realloc (cleaned, parent, j + 1);

I'm surprised that it's worth the effort to realloc, given that the returned string is fairly short-lived.

(I'll continue after lunch.)

« Back to merge proposal