Page MenuHomeFreeBSD

Fix CID 1307758: Unused value.
AbandonedPublic

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

Details

Summary

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

araujo retitled this revision from to Fix CID 1307758: Unused value..Jun 19 2015, 10:21 PM
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 6339.
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
usr.sbin/bhyvectl/bhyvectl.c
464

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

1560

This is wrong.

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

1571

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

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

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

Tks.

neel added inline comments.Jun 20 2015, 12:20 AM
usr.sbin/bhyvectl/bhyvectl.c
493

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;
else
        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.