I was wrong about the maximum file name length.  I didn't run into the path name
max, but the file name max (for one component of a file name).

For Linux, this is 255 bytes, which is easier to run into (e.g. with mix albumn
tracks that contain multiple songs).  I suppose it is even easier to run into
with  non-english tracks due to UTF-8 encoding.

Note: http://en.wikipedia.org/wiki/List_of_songs_with_particularly_long_titles

Created attachment 76461 [details] [review]
Adds a test case and shortens the filename if needed

I can confirm that this is a bug, even though the wikipedia link is no longer valid.  This patch checks to make sure that the string is less than 250 chars, and if it isn't, it gives an error dialog, and shortens it for the user.

Yeah, it got deleted. Still visible at http://en.wikipedia.org/wiki/User:Gbeeker/List_of_songs_with_particularly_long_titles

Thanks for the patch; looking at it now.

Problems:

1. You're assuming that 255 bytes is the filename length limit. This is fs-dependent; reiser4... well, OK, almost all filesystems have the 255-byte limit. In truth, no-one's going to be using a filesystem with a lesser limit (even vfat supports 255 bytes) and anyone using a filesystem with a longer limit is unlikely to care over much. But 255 should be made a #define constant.
2. You're assuming that the profile extension is a maximum of 4 bytes. Not necessarily the case; instead compare the file name to the maximum component length less the extension and '.'.
3. You're solving the wrong problem; it's each component of the path that needs to be at most 255 bytes, not the path itself. Split realfile on '/'s and truncate each excess component to 255 bytes or (for the last component) 255 bytes less the extension and '.'.
4. There is zero point notifying the user with an error dialog; there's nothing they can do about it. Write a message(s) to stderr (use g_warning etc.)
5. The error message could be better. Not your fault; your English is far better than my Italian. Correct the above points and I'll help with the translation.

As per the review by Ed, marking patch as needs-work.

I'm sorry, I'm somewhat new to programming in just C, and I've had some problems locating the correct function to split the string, and I would like to avoid "reinventing the wheel". I've seen strtok(), but the references I've seen say that's its not thread safe, and for other reasons, should generally be avoided.  

Is there a glib function I should be using?  Or should I just try to go through this by hand (scan string for /, memcpy() it to variable, move on, etc)?

g_strsplit(). See:
http://developer.gnome.org/doc/API/2.0/glib/glib-String-Utility-Functions.html#g-strsplit

Why do you need to, though? You should be working within filepath_parse_pattern(), where the path is constructed.

Also note MAXPATHLEN, typically 1024, although again the chances of hitting that are very slim.

Created attachment 81601 [details] [review]
Makes sure that we obey file system character limits

I haven't had a chance to test this, to be honest, and I wont for a while, but this should work;  Thanks to Ed Catmur for the help.

Instead of defining the path limit to be 1024, include limits.h and use PATH_MAX and NAME_MAX.  On Linux:

#define NAME_MAX         255    /* # chars in a file name */
#define PATH_MAX        4096    /* # chars in a path name including nul */

Note that on Hurd, as far as I recall, PATH_MAX is undefined as there isn't a maximum.

Created attachment 81674 [details] [review]
Use values from limits.h

I put the pathname limit in an #ifdef statement, going on what was said about GNU/Hurd not having it defined, because it didn't have the limitation.  Other than that, it should be good, now.

2007-02-01 Adam Petaccia <adam@tpetaccia.com>
	* src/sj-extracting.h:
	* src/sj-extracting.c:
	Don't write out files larger than the OS can handle

Comment on attachment 81674 [details] [review]
Use values from limits.h

You are creating a new GString for extension, then just accessing the ->str attribute, and then freeing it with g_free (which won't work).  Apart from the bug, why not just use a char*?

Yes, the gfree() part is a bug.  The reason for GString is to gain access to the ->len attribute, for determining the max length.  Also, as noted above, file extensions don't always have 3 letters + '.' (eg: .flac), so I thought it safer to do that, as getting the extension length just seemed more correct.

If you can fix the free bug and use a normal string with strlen(), I'll commit this.

Created attachment 87460 [details] [review]
Truncate the filename is needed

Sorry about the delay, this patch addresses the all bugs previously mentioned.  Thank you for the needed direction.

2007-05-03  Adam Petaccia <adam@tpetaccia.com>
	* src/sj-extracting.c:
	Don't silently fail if the filename is too long. Instead, truncate
	the filename.

The if PATHMAX block of code is wrong:

sj-extracting.c: In function ‘build_filename’:
sj-extracting.c:140: warning: passing argument 1 of ‘g_string_truncate’ from incompatible pointer type
sj-extracting.c:140: warning: assignment from incompatible pointer type

The line in question is:

+  uri = g_string_truncate (uri, PATH_MAX - 1 - tmp->len);

uri is a GnomeVFSURI, not a GString.

Also the PATH_MAX truncation happens after the extension is appended, which means that the extension will always be clobbered.  Instead can you check for a length that will exceed PATH_MAX before appending the extension?

Created attachment 102290 [details] [review]
sj-filelength.patch

Fixed up patch.

Great, please commit.

Committed, thanks.