Struct not assigning correctly

[FrostWraith]

I have a function to grab the mp3 id3v1 tag info from one of my songs and it writes it to a struct.  The problem is when I run a check on whether its an actual mp3 file (just check to see if bytes -128 through -126 are "TAG") it has written the actual header ("TAG") and the value of the title to the header, even though I specified it as char[3], as you will see in my struct.  Can anyone please see what I am doing wrong?

id3v1.header returns "TAGGet Up, Stand Up"

I also want to note that when I run sizeof id3v1.header it returns 3, even though that is contradictory to the string that is returned.

Code Select Expand

struct id3v1
{
char header[3];
char title[30];
char artist[30];
char album[30];
char year[4];
char comments[30];
char genre;
};

int loadtags(char *fname)
{
FILE* fh;
int ret;

fh = fopen(fname, READ_ONLY);

if (fh)
{
fseek(fh, -128, SEEK_END);
fread(&id3v1, sizeof id3v1, 1, fh);
fclose(fh);
printf("\n%s\n", id3v1.header);
ret = (id3v1.header == "TAG") ? 1 : 0; /* Check to see if it has an id3v1 tag */
}
else
{
ret = 0;
}
return ret;
}

[l2k-Shadow]

http://www.cplusplus.com/reference/clibrary/cstdio/fread.html

try

Code Select Expand


fread(&id3v1, sizeof(char), sizeof(id3v1), fh);


also, do remember that C strings are null-terminated. Therefore, if you assign TAG to your header[3], you miss the null-terminator since header is only 3 bytes long... so then if you ask it to print the header, it would print header,title,artist,etc... until it finds the null terminator in the memory.

also you can't check char arrays with if (header == "TAG").. use !strcmp(header, "TAG")

[FrostWraith]

Thank you for your reply.  The problem is that the tags in the file are not null terminated, instead they just go onto the next tag.  Now it just so happens that the title tag has some null characters in there which does end the string.  Since I am reading straight into the struct, is there an easy way to attach a null terminator to these strings?

[devcode]

try

Code Select Expand


fread(&id3v1, sizeof(char), sizeof(id3v1), fh);


His api call to fread is correct, so that's obviously not the problem (1 element of size id3v1). The only problem in his case is he needs to use strncmp( id3v1.header, "TAG", 3 ); although personally, I would use microsoft's stringsafe functions cause they're cooler.

[FrostWraith]

try

Code Select Expand


fread(&id3v1, sizeof(char), sizeof(id3v1), fh);


His api call to fread is correct, so that's obviously not the problem (1 element of size id3v1). The only problem in his case is he needs to use strncmp( id3v1.header, "TAG", 3 ); although personally, I would use microsoft's stringsafe functions cause they're cooler.

Not that I plan to use this on any unix variants, I strive for as much portability as possible.

[devcode]

try

Code Select Expand


fread(&id3v1, sizeof(char), sizeof(id3v1), fh);


His api call to fread is correct, so that's obviously not the problem (1 element of size id3v1). The only problem in his case is he needs to use strncmp( id3v1.header, "TAG", 3 ); although personally, I would use microsoft's stringsafe functions cause they're cooler.

Not that I plan to use this on any unix variants, I strive for as much portability as possible.

AFAIK, it's not windows dependent. <strsafe.h>, but I'm not completely sure since I don't do any personal development for platforms other than windope

[Yegg]

Note: Mac OS X has no strsafe.h. I've actually never heard of strsafe.h until you mentioned it. Since you mentioned it as Microsoft's stringsafe functions, why would they be available on other OS's? Windows Search also found no strsafe.h on my Windows XP.

[MyndFyre]

You should be using strcmp anyway.

Code Select Expand


char tagCmp[4];
const void* pHeader = (const void*)&(id3v1.header);  // this feels wrong, but you should get the idea
memcpy(&tagCmp, pHeader, 3);
tagCmp[3] = 0;
ret = (strcmp(&tagCmp, "TAG") == 0) ? 1 : 0;


[Win32]

Std lib is disgusting

[brew]

You should be using strcmp anyway.

Code Select Expand


char tagCmp[4];
const void* pHeader = (const void*)&(id3v1.header);  // this feels wrong, but you should get the idea
memcpy(&tagCmp, pHeader, 3);
tagCmp[3] = 0;
ret = (strcmp(&tagCmp, "TAG") == 0) ? 1 : 0;


Are you sure?

Code Select Expand


ID3    vPRIV     PeakValue ØF  PRIV     AverageLevel à   TCON      (9)TPE1   

I wouldn't know for certain, but based on what he's told us so far, and what I see in notepad (null characters are converted to spaces, and a box != a space ) it seems as if the tag he's talking about is just 3 bytes, not necissarily a string. So I, myself, would treat it as an array of three bytes, and use memcmp to check instead.

EDIT***

id3v1.header returns "TAGGet Up, Stand Up"

I also want to note that when I run sizeof id3v1.header it returns 3, even though that is contradictory to the string that is returned.

Not to mention that he copies the raw, binary data from the mp3 file to his struct
So if he does, why would he be missing anything? He said it returns TAGGet Up.... so on, if it were like what you think it should be in the binary data, wouldn't it be TAG no matter what? The first character of the title would be a null character and it'd stop reading right there. It would return that, logically, if you treat it as a string.
And when you run sizeof id3v1.header, of course it would return 3, that's the sizeof id3v1 header. perhaps you're thinking of strlen() (but that would not work properly anyways)
I'm a bit suprised nobody pointed all this out...

[MyndFyre]

The reason I did that is because strcmp is null-terminated (strncmp would have been another choice).

Using == is not the proper way to compare char[] in C.  The string literal "TAG" will always be in a static place in memory, as opposed to what he's reading from disk.  == will compare the memory locations for equality (unless he's using an STL std::string or MFC CString), which would only return true if they're the same reference, which we know they're not.

By doing it the way I suggested, he's able to use a standard library string compare function, we're sure that it's going to not run into any unchecked-input security issues, and he avoids the interned-string issue.

To respond to your other questions, the reason the header is "returning" "TAGGet Up, Stand Up" is because there isn't a null terminator at the end of "TAG" (as someone else already pointed out).  You could do a binary compare as well, but it's kind of awkward since it's only three bytes, which is why I opted for the copy-and-strcmp.

[Zorm]

The reason I did that is because strcmp is null-terminated (strncmp would have been another choice).

Using == is not the proper way to compare char[] in C.  The string literal "TAG" will always be in a static place in memory, as opposed to what he's reading from disk.  == will compare the memory locations for equality (unless he's using an STL std::string or MFC CString), which would only return true if they're the same reference, which we know they're not.

By doing it the way I suggested, he's able to use a standard library string compare function, we're sure that it's going to not run into any unchecked-input security issues, and he avoids the interned-string issue.

To respond to your other questions, the reason the header is "returning" "TAGGet Up, Stand Up" is because there isn't a null terminator at the end of "TAG" (as someone else already pointed out).  You could do a binary compare as well, but it's kind of awkward since it's only three bytes, which is why I opted for the copy-and-strcmp.

Why not just use memcmp? That would save you an extra copy...

Also while it looks like its not happening in your case, you'll probably eventually run into a situation where the compiler aligns a struct and you'll get nastiness where its not really the size you want.