Opened 7 years ago

Closed 7 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:
Has a Patch: yes Platform: All

Description

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

Attachments (3)

fs_func_c.patch (1.5 KB) - added by kag_anil 7 years ago.
fs_func_c_2.patch (1.8 KB) - added by kag_anil 7 years ago.
patch with modifications --> conforming to the guidelines and using B_READ_ONLY_DEVICE instead of B_OK for read only volumes
fs_func_c_final.patch (1.2 KB) - added by kag_anil 7 years ago.
Conforming to the suggestions made by bonefish

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by kag_anil

Attachment: fs_func_c.patch added

comment:1 Changed 7 years ago by kag_anil

Has a Patch: set

comment:2 Changed 7 years ago by 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.

Changed 7 years ago by kag_anil

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

comment:3 Changed 7 years ago by 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)

Last edited 7 years ago by kag_anil (previous) (diff)

comment:4 in reply to:  3 Changed 7 years ago by 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).

comment:5 Changed 7 years ago by 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.

comment:6 Changed 7 years ago by 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??

Last edited 7 years ago by kag_anil (previous) (diff)

Changed 7 years ago by kag_anil

Attachment: fs_func_c_final.patch added

Conforming to the suggestions made by bonefish

comment:7 Changed 7 years ago by korli

Resolution: fixed
Status: newclosed

Applied with fixes in hrev44325.

Note: See TracTickets for help on using tickets.