On 05/04/2011 06:23 AM, Laine Stump wrote: >>> This definitely fixes the FILE* leak, but do we really want to abort >>> the loop (stop looking for more pidfiles) when fscanf fails on one >>> pidfile? (dunno, just asking) Personally, I think that since pidfiles are always generated by the kernel, they should always have sane contents, so failure to read them means something is really wrong. The fact that the overall function returns -1, right away, rather than trying to read other pid files (if any), is in some regards a feature (if we're having problems reading one pid file, then what else is going wrong?). Furthermore, there were other places in the loop that aborted on the first failure, so this isn't the first case of aborting the outer loop if the inner loop fails. > I don't know if that's important or not, but it is a change in behavior. I've updated the commit message to call attention to the subtle (and unlikely) change in behavior. > >> ACK, but let's have a second patch for after 0.9.1 >> if we think this need improvement. I've pushed the patch unchanged for 0.9.1; right now, I don't think we need any post-0.9.1 improvement, but I'm still open to anyone else that can argue a case for trying to read remaining pid files even if we failed to read one. -- Eric Blake eblake@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list