On Feb 25, 2011, at 1:43 AM, Niv Sardi wrote:
> A bit of nitpiking:
>
> * indentation seems off.
What's the convention for this project? It looked like 4 spaces, and I tried
to match but might have let some tabs slip through. Someday I should learn how
to tweak vim to do this for me.
>> --- src/source.c (revision 17873)
>> +++ src/source.c (working copy)
>> @@ -714,6 +715,20 @@
>> }
>>
>> /* save stream to file */
>> + if (source->reopen_dumpfile) {
>> + if (source->dumpfile) {
>> + fclose(source->dumpfile);
>> + source->dumpfile = NULL;
>> + }
>
> * here you close a file, then check if there is a ->dumpfilename after
> you try to open a new one... if there is no ->dumpfilename (for
whatever
> reason, is it possible ?) I'd say the best is not to close the first
one.
It's possible if the config is changed and reloaded via HUP. The dumpfile
configuration parameter could be removed, and normally the file wouldn't be
closed until the source disconnected. With this change, a subsequent USR1 will
cause the file to be closed (bringing things into line with the new config).
The code also handles the opposite situation (B), where a dumpfile config
parameter is added to a mount that didn't have one previously.
To me, this all argues for the reopen of dumpfiles to be included in the HUP
handler. We're just activating these config parameters immediately instead
of waiting for the source to dis/re-connect.
>
>> + if (source->dumpfilename) {
>> + source->dumpfile =
fopen(source->dumpfilename, "ab");
>> + if (source->dumpfile == NULL) {
>> + WARN2("Cannot re-open dump file
\"%s\" for appending: %s, disabling.",
>> + source->dumpfilename,
strerror(errno));
>> + }
>> + }
>> + source->reopen_dumpfile = 0;
>> + }
>> if (source->dumpfile &&
source->format->write_buf_to_file)
>
> * and so with the coments above, you can do your magic here.
If I moved the reopen code here, I'd lose the added benefit (B) above.
>
>> source->format->write_buf_to_file (source,
refbuf);
>> }
>
> Cheers,
> --
> Niv Sardi
--
Ken Treis
Miriam Technologies, Inc.
