The logic here is weird, seems delim should be set before the printf.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
Comment Actions
- 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.
Comment Actions
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.
| 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'. | |
| 1561 | This is wrong. 'delim' should be '\0' until the first field is pretty-printed. After that is should be set to the real delimited '/'. | |
| 1572 | Huh? There *should* be a space before and after the '+=' operator. | |
Comment Actions
@neel thanks for the review.
Here is the correct patch then. Although I think, it is not worth
to commit this patch.
Tks.
| 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;
} | |