Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libc: Fix time() and implement gmtime() #21

Closed
wants to merge 5 commits into from
Closed

Conversation

sam-itt
Copy link

@sam-itt sam-itt commented Jan 23, 2020

No description provided.

* Original Author: Adapted from tzcode maintained by Arthur David Olson.
*
* This file is part of the Public Domain C Library (PDCLib).
* Permission is granted to use, modify, and / or redistribute at will.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is no longer true; copying from newlib is also problematic as it conflicts with the pdclib license and it's not clearly marked as such.

The file in newlib is actually problematic in itself because it doesn't have explicit copyright headers, but mentions a couple of contributors and external sources. Strictly speaking we'd have to add https://www.sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=COPYING.NEWLIB and also ship it with each release of an nxdk app - this feels overkill to me.

I guess we could say that some of these algorithms are fact and non-copyrightable (as are the constants), but it still feels "bad".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there even is a pointer in the comments to a github page that describe the algorithm. Being what it is (converting seconds since 1970 to day/month/etc) I don't think there are so many ways to do it.

@@ -9,13 +9,15 @@
#ifndef REGTEST

#include <xboxkrnl/xboxkrnl.h>
#define NT_EPOCH_TIME_OFFSET ((LONGLONG)(369 * 365 + 89) * 24 * 3600 * 10000000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this works (does it?). 3600 * 10000000 is more than 32-bit. I'd assume this has to be 3600LL * 10000000LL etc. (I keep forgetting about C int promotion rules, so maybe it's already fine?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works. That's from ntfs-3g ntfstime.h. The compiler uses the first type that fits. Better explained there: https://stackoverflow.com/questions/13134956/what-is-the-reason-for-explicitly-declaring-l-or-ul-for-long-values.

functions/time/gmtime.c Outdated Show resolved Hide resolved
functions/time/gmtime.c Outdated Show resolved Hide resolved
The offset between win32 and unix epochs wasn't taken care of,
resulting in a wrong value. Added the correct offset from ntfs3g.
Provide a working gmtime() implementation instead of the
NULL-returning pdclib stub.
Not resetting last seen '\n' offset after flushing the buffer
(which resets the buffer index to 0) can lead to a situation where
that offset has a higher value the the current buffer index,
overflowing (unsigned) stream->bufidx = bufidx - offset
and then doing a  memmove of a gigantic size.
@JayFoxRox
Copy link
Member

This includes the other changes now, none of which follow prefix style or have anything to do with this.
I think this might have been a mistake during a rebase?


Other than that I think the changes made the PR fine. I was hoping someone else would also review / merge this.

@sam-itt
Copy link
Author

sam-itt commented Jan 28, 2020

I think this might have been a mistake during a rebase?

I messed up. Here is a clean branch with only the intended changes: #24

@JayFoxRox
Copy link
Member

Oh! I see. Default branch for PR.

I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants