Page MenuHomeFreeBSD

Fix CID 1307758: Unused value.

Authored by araujo on Jun 19 2015, 10:21 PM.



The logic here is weird, seems delim should be set before the printf.

Diff Detail

rS FreeBSD src repository
Lint OK
No Unit Test Coverage

Event Timeline

araujo updated this revision to Diff 6339.Jun 19 2015, 10:21 PM
araujo retitled this revision from to Fix CID 1307758: Unused value..
araujo updated this object.
araujo edited the test plan for this revision. (Show Details)
araujo added reviewers: neel, rodrigc, grehan.
araujo updated this revision to Diff 6341.Jun 19 2015, 10:33 PM
  • It still doesn't looks right, I remake the patch and remove the '\0'.
  • Also initialize the variable map_size, CLANG complains about it.
  • Fix a minor style, an space at variable gpa.
neel edited edge metadata.Jun 19 2015, 10:55 PM

Thanks for the quick turnaround, but this idiom is intentional (I updated the CID a couple of hours ago).

The idea is that when pretty-printing the 'flags' value the first one should not have any delimiter (hence 'delim' is initialized to '\0'). However, any subsequent fields must be delimited by a '/' to distinguish them from the previous field.

It is technically correct that the last field that is pretty-printed does not need to set 'delim = /'. However, these blocks of code are usually copy-pasted so when more fields are added in the future this will continue to work correctly.

neel added inline comments.Jun 19 2015, 11:03 PM

clang is confused but if you want to fix this then is to move the initialization of 'map_size' before the first 'goto done'.


This is wrong.

'delim' should be '\0' until the first field is pretty-printed. After that is should be set to the real delimited '/'.


Huh? There *should* be a space before and after the '+=' operator.

araujo updated this revision to Diff 6343.Jun 19 2015, 11:34 PM
araujo edited edge metadata.

@neel thanks for the review.
Here is the correct patch then. Although I think, it is not worth
to commit this patch.


neel added inline comments.Jun 20 2015, 12:20 AM

This is introducing a bug in the common case where 'map_size' is getting reset to 0. Yes, this is not worth committing :-)

What I meant was to do this instead:

error = -1;
bitmap = MAP_FAILED;
if (cpu_intel)
        map_size = PAGE_SIZE;
        map_size = 2 * PAGE_SIZE;

fd = open("/dev/mem", O_RDONLY, 0);
if (fd < 0) {
        perror("Couldn't open /dev/mem");
        goto done;
araujo abandoned this revision.Jun 20 2015, 12:27 AM

Not worth to waste time on it, sorry guys.