GNOME Bugzilla – Bug 732020
Static analysis fixes
Last modified: 2014-06-22 00:22:36 UTC
Ran the current code through a static analyzer for the first time. There was only one major bug in WRT error handling of splice() on writing objects.
Created attachment 278909 [details] [review] [staticanalysis]: Fix two uses of uninitialized variables
Created attachment 278910 [details] [review] [staticanalysis]: Add missing va_end()
Created attachment 278911 [details] [review] [staticanalysis]: Add assertion to pacify analyzer This condition can't actually be hit, let's hint that's the case.
Created attachment 278912 [details] [review] [staticanalysis]: Actually check errors on splice() of objects We were using unsigned size when we should have been using signed, this means we basically weren't checking for errors on write...ouch. Luckily if we e.g. hit ENOSPC during a pull, the checksums wouldn't match and we'd return an error anyways. However when writing an object, we'd end up silently ignoring it =/
Created attachment 278913 [details] [review] [staticanalysis]: Fix some dead code One was an unused variable, the other is actually dead because we can't have mfile != NULL.
Created attachment 278914 [details] [review] [staticanalysis]: Fix in_status_line We need to end the status line *after* we've done a pull, as ostree admin upgrade does. Also add the correct in_status_line assignment.
Created attachment 278915 [details] [review] [staticanalysis]: Delete an unused variable
Review of attachment 278909 [details] [review]: Looks good.
Review of attachment 278910 [details] [review]: Yes.
Review of attachment 278911 [details] [review]: I did spend 10 minutes or so making sure this was actually the case. The code here can certainly be a lot cleaner.
Review of attachment 278912 [details] [review]: Looks good.
Review of attachment 278913 [details] [review]: "One, the other" -- there are actually three fixes here. Code looks good.
Review of attachment 278914 [details] [review]: OK. It would probably be a bit more convenient to simply have gs_console_end_status_line fizzle out if its in_status_line is FALSE, but that's a separate thing.
Review of attachment 278915 [details] [review]: Sure.
Thanks for the review! Yeah...the commit logic is hairy but hard to fix without breaking it into separate functions, which I don't think is worth the cost. Attachment 278909 [details] pushed as d706797 - [staticanalysis]: Fix two uses of uninitialized variables Attachment 278910 [details] pushed as dfda6e3 - [staticanalysis]: Add missing va_end() Attachment 278911 [details] pushed as 2dc0cea - [staticanalysis]: Add assertion to pacify analyzer Attachment 278912 [details] pushed as 5407998 - [staticanalysis]: Actually check errors on splice() of objects Attachment 278913 [details] pushed as 5936740 - [staticanalysis]: Fix some dead code Attachment 278914 [details] pushed as 4d04b14 - [staticanalysis]: Fix in_status_line Attachment 278915 [details] pushed as 40f99f7 - [staticanalysis]: Delete an unused variable