TODO taking read only volumes into account in src/add-ons/kernel/file_systems/ntfs/fs_func.c

[kag_anil]

In fs_func.c file in file_systems/ntfs/, a TODO about taking care of mounting a read only volume was left. I completed it via checking whether the device is raed only with the help of device_geometry class and then if it is read only i have given the mount flags same as that for "ns->ro

(flags & B_MOUNT_READ_ONLY) != 0".

[bonefish]

• You apparently missed that B_NO_ERROR == B_OK, that is your checkReadOnly() returns the same value regardless of whether the device is readable or writable.
• The name of checkReadOnly() doesn't agree with our coding style, nor does the name of the local variable rdOnlyCheck.
• There's no point in setting result, since the value is overwritten a few lines below anyway.
• If you solve a TODO, the TODO comment should be removed.
• Your patch introduces lines with trailing white space.
• Patches should be created with git format-patch.

[kag_anil]

patch with modifications --> conforming to the guidelines and using B_READ_ONLY_DEVICE instead of B_OK for read only volumes

[kag_anil]

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)

[korli]

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).

[bonefish]

• 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 be device, not dev.
• More coding style: ioctl() returns an int. An explicit check to convert it to a bool should be used in the if condition (ioctl(...) == 0).
• Helper function:
  • Should be static.
  • B_READ_ONLY_DEVICE is a flag. Returning it through a status_t is unusual to say the least. Since you're not interested in the error code anyway, check_read_only() should be named is_read_only() (or is_device_read_only()) and return a bool. That also makes the local variable in fs_mount() superfluous.
  • The function should return early in case open() fails. That saves one if level.
  • check should be bool and named isReadOnly. 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.

[kag_anil]

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??

[kag_anil]

Conforming to the suggestions made by bonefish

[korli]

Applied with fixes in hrev44325.