Closed Bug 1072922 Opened 5 years ago Closed 5 years ago

fcntl constants and flock strutured to OS.File.Constantcs.libc

Categories

(Toolkit :: OS.File, defect)

33 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: noitidart, Assigned: harshil158, Mentored)

Details

(Whiteboard: [lang=c++][good first bug])

Attachments

(1 file, 6 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20140922173023

Steps to reproduce:

Created fcntl in js-ctypes: https://gist.github.com/Noitidart/f05ffde7dcaffe792a4d




Actual results:

Found that constants vary per OS


Expected results:

so was recommended to add constants to libc.
Whiteboard: [lang=c++][good first bug]
Component: Untriaged → OS.File
Product: Firefox → Toolkit
noitidart: You might want to read this
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

I'm not a developer but i will try to help if you have questions
hello sir, 
          i am interested in contributing to this bug.I am very much new here....can you guide me getting started with firefox and with this bug?
(In reply to noitidart from comment #1)
> patch ready:
> 
> https://gist.github.com/Noitidart/2c53244c2d65acec518a

Could you try creating an actual patch from this? There's instructions here:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I...

on how to create a patch file based on your changes to the source dir.
Flags: needinfo?(noitidart)
Ill try thanks. I got some lab work to do today, lab tomorrow from 430-930.

If pranali wants to take this im cool. I have another bug im thinking of filing and contributing on.
Flags: needinfo?(noitidart)
Can someone explain me what we have to do in this bug and tell me if anybody has been assigned or not. If not then it would be grateful that somebody guide me into fixing it as this would be my first bug.
Thanks
Hi Harshil. I don't think anyone was assign, I was trying to do this but I got real busy with helping others with add-ons and trying to get my own add-ons supported for e10 etc and then school and work.

Basically this brings support for flock.

See the gist above thats basically all that needs to go in.
I would like to work on the code, how to get is assigned?
Attachment #8524063 - Attachment is obsolete: true
Assignee: nobody → harshil158
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8524095 - Flags: review?(dteller)
Comment on attachment 8524095 [details] [diff] [review]
"Add libc constants for flock structure type."

Review of attachment 8524095 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, if it compiles on all platforms.
Have you already run this through the Try server?
Attachment #8524095 - Flags: review?(dteller) → review+
Oh, I forgot: could you change your commit message to add ",r=Yoric" at the end of the title?
Yes i will add that, and I have only ran it on my machine and it was working fine.(the offset values were as expected)
Attachment #8524095 - Attachment is obsolete: true
Attachment #8524678 - Flags: review?(dteller)
Comment on attachment 8524678 [details] [diff] [review]
Add libc constants for flock structure type,

Review of attachment 8524678 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Let's see how it works on the Try Server: https://hg.mozilla.org/try/pushloghtml?changeset=84f768a4657c
Attachment #8524678 - Flags: review?(dteller) → review+
Sorry, that was meant to be https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eacc77b3aa9a

As you can see, it won't build under Windows :/
Umm, what to do then?
I believe that your changes don't make sense under Windows, so they should be wrapped in a `#if defined(XP_UNIX) / #endif`.
Yeah this is only for mac and linux'es.

what does XP in the XP_UNIX stand for? i alwasy thought it mean winXP or Unix
XP = Cross Platform. That's older than Windows XP.
Okay, but wont it work only if there was no problem in windows in first place.
Oh cool thanks yoric man!
Okay, but wont that work only if there was no problem in windows in first place.
There was no problem in Windows. This is a new feature that can work only under Unix.
Attachment #8524678 - Attachment is obsolete: true
Attachment #8526231 - Flags: review?(dteller)
Attachment #8526231 - Attachment is obsolete: true
Attachment #8526231 - Flags: review?(dteller)
Attachment #8526241 - Flags: review?(dteller)
Comment on attachment 8526241 [details] [diff] [review]
Add libc constants for flock structure type,

Review of attachment 8526241 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/OSFileConstants.cpp
@@ +480,5 @@
>    INT_CONSTANT(SEEK_END),
>    INT_CONSTANT(SEEK_SET),
>  
> + // fcntl command values
> +  #if defined(XP_UNIX)

No space before #if or #endif.

@@ +481,5 @@
>    INT_CONSTANT(SEEK_SET),
>  
> + // fcntl command values
> +  #if defined(XP_UNIX)
> +    INT_CONSTANT(F_GETLK),

Please keep the indentation of the rest of the file.

@@ +489,5 @@
> + // flock type values
> +    INT_CONSTANT(F_RDLCK),
> +    INT_CONSTANT(F_WRLCK),
> +    INT_CONSTANT(F_UNLCK),
> +  #endif  

No whitespace at the end of the line.

@@ +597,5 @@
>    // Size
>    { "OSFILE_SIZEOF_DIRENT", INT_TO_JSVAL(sizeof (dirent)) },
>  
> +  // Defining |flock|.
> +  #if defined(XP_UNIX)

Same remarks as above.
Attachment #8526241 - Flags: review?(dteller) → feedback+
Attachment #8526241 - Attachment is obsolete: true
Attachment #8526263 - Flags: review?(dteller)
Comment on attachment 8526263 [details] [diff] [review]
Add libc constants for flock structure type,

Review of attachment 8526263 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there.

::: dom/system/OSFileConstants.cpp
@@ +489,5 @@
> + // flock type values
> +  INT_CONSTANT(F_RDLCK),
> +  INT_CONSTANT(F_WRLCK),
> +  INT_CONSTANT(F_UNLCK),
> +#endif  

Nit: No whitespace at the end of the line.
On the other hand, if you look at the rest of the file, this should be
#endif // defined(XP_UNIX)

@@ +604,5 @@
> +  { "OSFILE_OFFSETOF_FLOCK_L_LEN", INT_TO_JSVAL(offsetof (struct flock, l_len)) },
> +  { "OSFILE_OFFSETOF_FLOCK_L_PID", INT_TO_JSVAL(offsetof (struct flock, l_pid)) },
> +  { "OSFILE_OFFSETOF_FLOCK_L_TYPE", INT_TO_JSVAL(offsetof (struct flock, l_type)) },
> +  { "OSFILE_OFFSETOF_FLOCK_L_WHENCE", INT_TO_JSVAL(offsetof (struct flock, l_whence)) },
> +#endif

Same here.
Attachment #8526263 - Flags: review?(dteller) → feedback+
Attachment #8526263 - Attachment is obsolete: true
Attachment #8526476 - Flags: review?(dteller)
Comment on attachment 8526476 [details] [diff] [review]
Add libc constants for flock structure type,

Review of attachment 8526476 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Let's see if it works as expected: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d90e1a71794
Attachment #8526476 - Flags: review?(dteller) → review+
Tests look good. Here we go.
Keywords: checkin-needed
What does `#if defined(XP_UNIX)` do if it encounter Mac OS X? Because this is also for Mac.
https://hg.mozilla.org/integration/fx-team/rev/a7bd9b15a071
Keywords: checkin-needed
Whiteboard: [lang=c++][good first bug] → [lang=c++][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a7bd9b15a071
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=c++][good first bug][fixed-in-fx-team] → [lang=c++][good first bug]
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.