Opened 13 years ago
Closed 12 years ago
#8449 closed enhancement (fixed)
TODO taking read only volumes into account in src/add-ons/kernel/file_systems/ntfs/fs_func.c
Reported by: | kag_anil | Owned by: | 3dEyes |
---|---|---|---|
Priority: | normal | Milestone: | R1 |
Component: | File Systems/NTFS | Version: | R1/alpha3 |
Keywords: | NTFS, gsoc2012 | Cc: | |
Blocked By: | Blocking: | ||
Platform: | All |
Description
(flags & B_MOUNT_READ_ONLY) != 0". |
Attachments (3)
Change History (10)
by , 13 years ago
Attachment: | fs_func_c.patch added |
---|
comment:1 by , 13 years ago
patch: | 0 → 1 |
---|
comment:2 by , 13 years ago
by , 13 years ago
Attachment: | fs_func_c_2.patch added |
---|
patch with modifications --> conforming to the guidelines and using B_READ_ONLY_DEVICE instead of B_OK for read only volumes
follow-up: 4 comment:3 by , 13 years ago
1) yups i have missed the B_NO_ERROR == B_OK fact, now i'm using B_READ_ONLY_DEVICE for read only device
2) now i've used the space_separated identifiers(but haiku's guidelines says about using interCapsFormatting)
3) i've removed the result = rd_only_check, as u have pointed out it is overwritten anyway some lines next to it
4) now i've removed the TODO comment
5) i've used the git diff command(sorry i was not able to use the git format-patch)
comment:4 by , 13 years ago
Replying to kag_anil:
5) i've used the git diff command(sorry i was not able to use the git format-patch)
git commit has to be used before git format-patch. By default it generates a patch file (nothing on the standard output).
comment:5 by , 13 years ago
- Camel case for local variables was fine. What didn't and still doesn't agree with the coding style is the use of uncommon abbreviations.
readOnlyCheck
would be correct. And the helper function's parameter should bedevice
, notdev
. - More coding style:
ioctl()
returns an int. An explicit check to convert it to a bool should be used in theif
condition (ioctl(...) == 0
). - Helper function:
- Should be static.
B_READ_ONLY_DEVICE
is a flag. Returning it through astatus_t
is unusual to say the least. Since you're not interested in the error code anyway,check_read_only()
should be namedis_read_only()
(oris_device_read_only()
) and return a bool. That also makes the local variable infs_mount()
superfluous.- The function should return early in case
open()
fails. That saves one if level. check
should be bool and namedisReadOnly
. That saves the innermost if.- While comments on the same line after the actual code are not disallowed per se, they should generally be avoided (cf. the coding guidelines). In this case the three comments don't add any value anyway and can just be omitted.
comment:6 by , 13 years ago
i'm going for
if(ioctl(fd, B_GET_GEOMETRY, &geometry) == 0), as ioctl() returns 0 on success and -1 on failure, but i'm not sure about the other cases in which a non-negative value is returned as an output parameter. Should i go with it??
by , 13 years ago
Attachment: | fs_func_c_final.patch added |
---|
Conforming to the suggestions made by bonefish
comment:7 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Applied with fixes in hrev44325.
B_NO_ERROR == B_OK
, that is yourcheckReadOnly()
returns the same value regardless of whether the device is readable or writable.checkReadOnly()
doesn't agree with our coding style, nor does the name of the local variablerdOnlyCheck
.result
, since the value is overwritten a few lines below anyway.git format-patch
.