From: Tilman Sauerbeck Date: Mon, 14 Aug 2006 20:04:56 +0000 (+0200) Subject: Reworked output file writing. X-Git-Url: http://git.code-monkey.de/?p=ruby-vorbistagger.git;a=commitdiff_plain;h=fcd91ff1a3cc4a4f889abfd2af89119cbe262ad1 Reworked output file writing. Ogg::Vorbis::Tagger can only operate on files now, since read-write support cannot be implemented properly with streams. This allowed fixing a corruption bug, that would break Ogg Vorbis files when writing comments to them. --- diff --git a/ext/ext.c b/ext/ext.c index da2798f..0ee20d3 100644 --- a/ext/ext.c +++ b/ext/ext.c @@ -17,61 +17,22 @@ */ #include +#include #include +#include #include "vcedit.h" #include "comments.h" typedef struct { - VALUE io; - bool need_close; - vcedit_state *state; - VALUE comments; } RbVorbisTagger; static VALUE c_close (VALUE self); -static VALUE cComments, eVTError, io_buf; -static ID id_read, id_write, id_seek, id_length; - -static size_t -on_read (void *ptr, size_t size, size_t nmemb, RbVorbisTagger *o) -{ - struct RString *buf; - size_t total = size * nmemb; - VALUE tmp; - - rb_str_resize (io_buf, size * nmemb); - - tmp = rb_funcall (o->io, id_read, 2, LONG2NUM (total), io_buf); - if (NIL_P (tmp)) - return 0; - - buf = RSTRING (tmp); - memcpy (ptr, buf->ptr, buf->len); - - return buf->len; -} - -static size_t -on_write (const void *ptr, size_t size, size_t nmemb, RbVorbisTagger *o) -{ - size_t total = size * nmemb; - - rb_str_resize (io_buf, total); - memcpy (RSTRING (io_buf)->ptr, ptr, total); - - return NUM2LONG (rb_io_write (o->io, io_buf)); -} - -static void -c_mark (RbVorbisTagger *o) -{ - rb_gc_mark (o->io); - rb_gc_mark (o->comments); -} +static VALUE cComments, eVTError; +static ID id_length; static void c_free (RbVorbisTagger *o) @@ -83,6 +44,12 @@ c_free (RbVorbisTagger *o) ruby_xfree (o); } +static void +c_mark (RbVorbisTagger *o) +{ + rb_gc_mark (o->comments); +} + static VALUE c_alloc (VALUE klass) { @@ -94,14 +61,12 @@ c_alloc (VALUE klass) /* * call-seq: * Ogg::Vorbis::Tagger.open(arg) -> object - * Ogg::Vorbis::Tagger.open(arg) { |object| block } -> nil + * Ogg::Vorbis::Tagger.open(arg) { |object| block } -> object * * If a block isn't specified, Ogg::Vorbis::Tagger.open is a synonym * for Ogg::Vorbis::Tagger.new. - * If a block is given, it will be invoked with the - * Ogg::Vorbis::Tagger object as a parameter, and the file or IO object - * will be automatically closed when the block terminates. - * The method always returns +nil+ in this case. + * If a block is given, it will be invoked with the * Ogg::Vorbis::Tagger + * object as a parameter. */ static VALUE c_open (VALUE klass, VALUE arg) @@ -116,47 +81,28 @@ c_open (VALUE klass, VALUE arg) /* * call-seq: - * Ogg::Vorbis::Tagger.new(arg) -> object + * Ogg::Vorbis::Tagger.new(filename) -> object * - * Returns a new Ogg::Vorbis::Tagger object for the specified argument. - * *arg* can either be an IO object or a filename. + * Returns a new Ogg::Vorbis::Tagger object for the specified file. * * FIXME: add optional mode argument (read-only or read-write) */ static VALUE -c_init (VALUE self, VALUE io) +c_init (VALUE self, VALUE filename) { RbVorbisTagger *o; vorbis_comment *vc; - int s, i; + int i; Data_Get_Struct (self, RbVorbisTagger, o); - /* is this actually an IO object or a filename? */ - if (rb_respond_to (io, id_read) && - rb_respond_to (io, id_write) && - rb_respond_to (io, id_seek)) - o->need_close = false; - else if (!NIL_P (rb_check_string_type (io))) { - io = rb_file_open (StringValuePtr (io), "rb+"); - o->need_close = true; - } else - rb_raise (rb_eArgError, "invalid argument"); + StringValue (filename); - o->io = io; - - o->state = vcedit_state_new (); + o->state = vcedit_state_new (StringValuePtr (filename)); if (!o->state) rb_raise (eVTError, "vcedit_new_state() failed - %s", vcedit_error (o->state)); - s = vcedit_open_callbacks (o->state, o, - (vcedit_read_func) on_read, - (vcedit_write_func) on_write); - if (s < 0) - rb_raise (eVTError, "vcedit_open_callbacks() failed - %s", - vcedit_error (o->state)); - vc = vcedit_comments (o->state); if (!vc) rb_raise (eVTError, "vcedit_comments() failed - %s", @@ -194,9 +140,6 @@ c_close (VALUE self) vcedit_state_unref (o->state); o->state = NULL; - if (o->need_close) - rb_io_close (o->io); - return self; } @@ -211,17 +154,12 @@ static VALUE c_write (VALUE self) { RbVorbisTagger *o; - int s; Data_Get_Struct (self, RbVorbisTagger, o); comments_sync (o->comments, o->state); - /* seek back to BOF */ - rb_funcall (o->io, id_seek, 1, INT2FIX (0)); - - s = vcedit_write (o->state, o); - if (s < 0) + if (vcedit_write (o->state) < 0) rb_raise (rb_eIOError, "write failed - %s", vcedit_error (o->state)); @@ -263,19 +201,12 @@ Init_vorbistagger_ext (void) rb_define_singleton_method (cVT, "open", c_open, 1); rb_define_method (cVT, "initialize", c_init, 1); - rb_define_method (cVT, "close", c_close, 0); rb_define_method (cVT, "write", c_write, 0); rb_define_method (cVT, "comments", c_comments, 0); eVTError = rb_define_class_under (cVT, "TaggerError", eVorbis); - id_read = rb_intern ("read"); - id_write = rb_intern ("write"); - id_seek = rb_intern ("seek"); id_length = rb_intern ("length"); cComments = Init_Comments (mVorbis); - - io_buf = rb_str_buf_new (0); - rb_global_variable (&io_buf); } diff --git a/ext/vcedit.c b/ext/vcedit.c index c7bd8e7..b6c7dfd 100644 --- a/ext/vcedit.c +++ b/ext/vcedit.c @@ -20,13 +20,18 @@ #include #include #include +#include +#include #include #include +#include #include "vcedit.h" #define CHUNKSIZE 4096 +static int vcedit_open (vcedit_state *state); + struct vcedit_state_St { int refcount; @@ -36,10 +41,9 @@ struct vcedit_state_St { vorbis_comment *vc; vorbis_info *vi; - vcedit_read_func read; - vcedit_write_func write; + char filename[PATH_MAX]; - void *in; + FILE *in; long serial; unsigned char *mainbuf; unsigned char *bookbuf; @@ -53,7 +57,7 @@ struct vcedit_state_St { }; vcedit_state * -vcedit_state_new (void) +vcedit_state_new (const char *filename) { vcedit_state *state; @@ -65,6 +69,16 @@ vcedit_state_new (void) state->refcount = 1; + snprintf (state->filename, sizeof (state->filename), + "%s", filename); + + state->in = fopen (state->filename, "rb"); + + if (vcedit_open (state) < 0) { + free (state); + return NULL; + } + return state; } @@ -83,35 +97,43 @@ vcedit_comments (vcedit_state *state) static void vcedit_clear_internals (vcedit_state *state) { - const char *tmp; - if (state->vc) { vorbis_comment_clear (state->vc); free (state->vc); + state->vc = NULL; } if (state->os) { ogg_stream_clear (state->os); free (state->os); + state->os = NULL; } if (state->oy) { ogg_sync_clear (state->oy); free (state->oy); + state->oy = NULL; } free (state->vendor); + state->vendor = NULL; + free (state->mainbuf); + state->mainbuf = NULL; + state->mainlen = 0; + free (state->bookbuf); + state->bookbuf = NULL; + state->booklen = 0; - if (state->vi) { + if (state->vi) { vorbis_info_clear (state->vi); - free (state->vi); - } + free (state->vi); + state->vi = NULL; + } - tmp = state->lasterror; - memset (state, 0, sizeof (vcedit_state)); - state->lasterror = tmp; + state->serial = 0; + state->prevW = state->extrapage = state->eosin = 0; } void @@ -126,6 +148,7 @@ vcedit_state_unref (vcedit_state *state) state->refcount--; if (!state->refcount) { + fclose (state->in); vcedit_clear_internals (state); free (state); } @@ -220,7 +243,7 @@ _fetch_next_packet (vcedit_state *s, ogg_packet *p, ogg_page *page) while (ogg_sync_pageout (s->oy, page) <= 0) { buffer = ogg_sync_buffer (s->oy, CHUNKSIZE); - bytes = s->read (buffer, 1, CHUNKSIZE, s->in); + bytes = fread (buffer, 1, CHUNKSIZE, s->in); ogg_sync_wrote (s->oy, bytes); if (!bytes) @@ -240,10 +263,8 @@ _fetch_next_packet (vcedit_state *s, ogg_packet *p, ogg_page *page) return _fetch_next_packet (s, p, page); } -int -vcedit_open_callbacks (vcedit_state *state, void *in, - vcedit_read_func read_func, - vcedit_write_func write_func) +static int +vcedit_open (vcedit_state *state) { char *buffer; int bytes, i; @@ -252,16 +273,12 @@ vcedit_open_callbacks (vcedit_state *state, void *in, ogg_packet header_main, header_comments, header_codebooks; ogg_page og; - state->in = in; - state->read = read_func; - state->write = write_func; - state->oy = malloc (sizeof (ogg_sync_state)); ogg_sync_init (state->oy); while (1) { buffer = ogg_sync_buffer (state->oy, CHUNKSIZE); - bytes = state->read (buffer, 1, CHUNKSIZE, state->in); + bytes = fread (buffer, 1, CHUNKSIZE, state->in); ogg_sync_wrote (state->oy, bytes); @@ -348,7 +365,7 @@ vcedit_open_callbacks (vcedit_state *state, void *in, } buffer = ogg_sync_buffer (state->oy, CHUNKSIZE); - bytes = state->read (buffer, 1, CHUNKSIZE, state->in); + bytes = fread (buffer, 1, CHUNKSIZE, state->in); if (bytes == 0 && i < 2) { state->lasterror = "EOF before end of vorbis headers."; @@ -371,16 +388,36 @@ err: } int -vcedit_write (vcedit_state *state, void *out) +vcedit_write (vcedit_state *state) { ogg_stream_state streamout; ogg_packet header_main, header_comments, header_codebooks, op; ogg_page ogout, ogin; ogg_int64_t granpos = 0; - int result, bytes, needflush = 0, needout = 0; - char *buffer; + FILE *out; + char *buffer, tmpfile[PATH_MAX]; + int s, result, bytes, needflush = 0, needout = 0; size_t tmp; + strcpy (tmpfile, state->filename); + strcat (tmpfile, ".XXXXXX"); + + s = mkstemp (tmpfile); + if (s == -1) { + state->lasterror = "Error writing stream to output. " + "Cannot open temporary file."; + return -1; + } + + out = fdopen (s, "wb"); + if (!out) { + unlink (tmpfile); + close (s); + state->lasterror = "Error writing stream to output. " + "Cannot open temporary file."; + return -1; + } + state->eosin = 0; state->extrapage = 0; @@ -405,11 +442,11 @@ vcedit_write (vcedit_state *state, void *out) ogg_stream_packetin (&streamout, &header_codebooks); while ((result = ogg_stream_flush (&streamout, &ogout))) { - tmp = state->write (ogout.header, 1, ogout.header_len, out); + tmp = fwrite (ogout.header, 1, ogout.header_len, out); if (tmp != (size_t) ogout.header_len) goto cleanup; - tmp = state->write (ogout.body, 1, ogout.body_len, out); + tmp = fwrite (ogout.body, 1, ogout.body_len, out); if (tmp != (size_t) ogout.body_len) goto cleanup; } @@ -422,21 +459,21 @@ vcedit_write (vcedit_state *state, void *out) if (needflush) { if (ogg_stream_flush (&streamout, &ogout)) { - tmp = state->write (ogout.header, 1, ogout.header_len, out); + tmp = fwrite (ogout.header, 1, ogout.header_len, out); if (tmp != (size_t) ogout.header_len) goto cleanup; - tmp = state->write (ogout.body, 1, ogout.body_len, out); + tmp = fwrite (ogout.body, 1, ogout.body_len, out); if (tmp != (size_t) ogout.body_len) goto cleanup; } } else if (needout) { if (ogg_stream_pageout (&streamout, &ogout)) { - tmp = state->write (ogout.header, 1, ogout.header_len, out); + tmp = fwrite (ogout.header, 1, ogout.header_len, out); if (tmp != (size_t) ogout.header_len) goto cleanup; - tmp = state->write (ogout.body, 1, ogout.body_len, out); + tmp = fwrite (ogout.body, 1, ogout.body_len, out); if (tmp != (size_t) ogout.body_len) goto cleanup; } @@ -465,21 +502,21 @@ vcedit_write (vcedit_state *state, void *out) streamout.e_o_s = 1; while (ogg_stream_flush (&streamout, &ogout)) { - tmp = state->write (ogout.header, 1, ogout.header_len, out); + tmp = fwrite (ogout.header, 1, ogout.header_len, out); if (tmp != (size_t) ogout.header_len) goto cleanup; - tmp = state->write (ogout.body, 1, ogout.body_len, out); + tmp = fwrite (ogout.body, 1, ogout.body_len, out); if (tmp != (size_t) ogout.body_len) goto cleanup; } if (state->extrapage) { - tmp = state->write (ogin.header, 1, ogin.header_len, out); + tmp = fwrite (ogin.header, 1, ogin.header_len, out); if (tmp != (size_t) ogin.header_len) goto cleanup; - tmp = state->write (ogin.body, 1, ogin.body_len, out); + tmp = fwrite (ogin.body, 1, ogin.body_len, out); if (tmp != (size_t) ogin.body_len) goto cleanup; } @@ -503,18 +540,18 @@ vcedit_write (vcedit_state *state, void *out) /* Don't bother going through the rest, we can just * write the page out now */ - tmp = state->write (ogout.header,1,ogout.header_len, out); + tmp = fwrite (ogout.header, 1, ogout.header_len, out); if (tmp != (size_t) ogout.header_len) goto cleanup; - tmp = state->write (ogout.body,1,ogout.body_len, out); + tmp = fwrite (ogout.body, 1, ogout.body_len, out); if (tmp != (size_t) ogout.body_len) goto cleanup; } } buffer = ogg_sync_buffer (state->oy, CHUNKSIZE); - bytes = state->read (buffer, 1, CHUNKSIZE, state->in); + bytes = fread (buffer, 1, CHUNKSIZE, state->in); ogg_sync_wrote (state->oy, bytes); if (!bytes) { @@ -523,6 +560,12 @@ vcedit_write (vcedit_state *state, void *out) } } + fclose (out); + fclose (state->in); + + unlink (state->filename); + rename (tmpfile, state->filename); + cleanup: ogg_stream_clear (&streamout); @@ -542,5 +585,10 @@ cleanup: return -1; } + vcedit_clear_internals (state); + + state->in = fopen (state->filename, "rb"); + vcedit_open (state); + return 0; } diff --git a/ext/vcedit.h b/ext/vcedit.h index a8aa33c..902d14e 100644 --- a/ext/vcedit.h +++ b/ext/vcedit.h @@ -27,19 +27,13 @@ extern "C" { #include #include -typedef size_t (*vcedit_read_func)(void *, size_t, size_t, void *); -typedef size_t (*vcedit_write_func)(const void *, size_t, size_t, void *); - typedef struct vcedit_state_St vcedit_state; -vcedit_state *vcedit_state_new (void); +vcedit_state *vcedit_state_new (const char *filename); void vcedit_state_ref (vcedit_state *state); void vcedit_state_unref (vcedit_state *state); vorbis_comment *vcedit_comments (vcedit_state *state); -int vcedit_open_callbacks (vcedit_state *state, void *in, - vcedit_read_func read_func, - vcedit_write_func write_func); -int vcedit_write (vcedit_state *state, void *out); +int vcedit_write (vcedit_state *state); const char *vcedit_error (vcedit_state *state); #ifdef __cplusplus diff --git a/test/test_main.rb b/test/test_main.rb index ae30ded..21c4edf 100644 --- a/test/test_main.rb +++ b/test/test_main.rb @@ -1,7 +1,6 @@ require "test/unit" require "ogg/vorbis/tagger" require "fileutils" -require "stringio" require "open3" class MainTest < Test::Unit::TestCase @@ -12,7 +11,7 @@ class MainTest < Test::Unit::TestCase -c "artist=Bolt Thrower" -c "album=...For Victory" -c "date=1994" EOF - @ogg_buf = StringIO.new + f = File.open(OGG_FILE, "w") cmd = "oggenc -q 0 #{tmp.strip} -r -" Open3.popen3(cmd) do |sin, sout, _| @@ -21,19 +20,19 @@ EOF begin tmp = sout.read - @ogg_buf << tmp + f << tmp end while !tmp.length.zero? end - @ogg_buf.seek(0) + f.close end def teardown - @ogg_buf.close + FileUtils.rm_f(OGG_FILE) end def test_read - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| # make sure the keys are returned in the correct order assert_equal(["artist", "album", "date"], t.comments.keys) assert_equal(["Bolt Thrower", "...For Victory", "1994"], @@ -49,72 +48,62 @@ EOF end def test_write_stable_order - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert_equal(3, t.write) end - @ogg_buf.seek(0) - - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert_equal(["artist", "album", "date"], t.comments.keys) end end def test_write_stable_order_change - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| t.comments["artist"] = "Ballista" assert_equal(3, t.write) end - @ogg_buf.seek(0) - - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert_equal(["artist", "album", "date"], t.comments.keys) end end def test_append - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| t.comments["genre"] = "Death Metal" assert_equal(4, t.write) end - @ogg_buf.seek(0) - - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert_equal("Death Metal", t.comments["genre"]) end end def test_delete - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert_equal("...For Victory", t.comments.delete("album")) assert_nil(t.comments.delete("foo")) assert_equal(2, t.write) end - @ogg_buf.seek(0) - - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert_equal(["artist", "date"], t.comments.keys) end end def test_clear - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| t.comments.clear assert_equal(0, t.write) end - @ogg_buf.seek(0) - - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert(t.comments.empty?) end end def test_empty - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert(!t.comments.empty?) t.comments.delete("artist") @@ -124,15 +113,13 @@ EOF assert_equal(0, t.write) end - @ogg_buf.seek(0) - - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert(t.comments.empty?) end end def test_each - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| a = { "artist" => "Bolt Thrower", "album" => "...For Victory", @@ -149,7 +136,7 @@ EOF end def test_each_key - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| b = [] t.comments.each_key do |k| @@ -161,7 +148,7 @@ EOF end def test_each_value - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| b = [] t.comments.each_value do |v| @@ -173,7 +160,7 @@ EOF end def test_inspect - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| tmp=<"Bolt Thrower", "album"=>"...For Victory", "date"=>"1994"} EOF @@ -182,7 +169,7 @@ EOF end def test_has_key - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert(t.comments.has_key?("artist")) assert(!t.comments.has_key?("foo")) @@ -196,13 +183,11 @@ EOF a = nil b = nil - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| a = t.comments end - @ogg_buf.seek(0) - - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| b = t.comments end @@ -212,7 +197,7 @@ EOF end def test_modify_existing_key - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| assert_raises(TypeError) do t.comments.keys.first.replace("new") end @@ -220,7 +205,7 @@ EOF end def test_modify_added_key - Ogg::Vorbis::Tagger.open(@ogg_buf) do |t| + Ogg::Vorbis::Tagger.open(OGG_FILE) do |t| t.comments["Foo"] = "Bar" assert_raises(TypeError) do