Hi,
   I have  a c++ code which is a message queue.


 Here is the code for enqueue:


##########################################################
Enqueue
##########################################################
bool FileMessageQueue::Enqueue( const string& in_message )
{
   I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );



   bool out = false;


   ofstream sout( _fileName.c_str(), ios::app | ios::ate );


   if ( true == sout.good() )
   {
      sout << in_message << endl;
   }
   else
   {
      I_LOG( LM_ERROR, "FileMessageQueue - failed opening output stream

for enqueue on file: '%s' for message: '%s'.\n", _fileName.c_str(),
in_message.c_str() );
   }


   out = sout.good();


   sout.close();


   return out;



}


  #######################################################
How can I make this enqueue to write to disk(synchronise it)?
Any suggestions would be very helpful.


Thanks


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

kavi81@lycos.com wrote:

> How can I make this enqueue to write to disk(synchronise it)?
> Any suggestions would be very helpful.
>

Please explain what do you need with "write to disk(synchronise it)".
Really that's obscure to me. Write what? Syncronise what? The code *is*
indeed writing something to disk... so what are you trying to achieve,
exactly? If you mean "flushing" the stream buffer, then closing the
stream should already be enough. If you mean physically writing to disk
(in case there's a disk cache, for example) that's an O/S detail and
it's not covered by the C++ language; there's no portable way to achieve
that.

Anyway, I can give you a few style advices regarding iostream usage.
It's my personal style so take them for what's worth:

1) avoid using ios::ate: although it's standard, it's implemented
differently by different O/S. In your case ios::app already provides
what you need (appending at the end of a text file), so you don't need
ios::ate anyway.

2) usually you don't use good() to check the stream status, but use
fail() instead. The first will return false on eof, and eof is usually
*not* a failure. As in your case it's an output stream, it doesn't
really make a difference, but for an istream it might. I usually prefer
to be consistent between the input and output case.

3) the stream destructor will automatically close the stream, so you
don't need to call close() explictly.

For example, if I were to write your code, I would write it like this:

bool FileMessageQueue::Enqueue( const string& in_message )
{
   I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );
   ofstream sout( _fileName.c_str(), ios::app );
   if ( sout.fail()  )
   {
      I_LOG( LM_ERROR, "FileMessageQueue - failed opening output stream
 for enqueue on file: '%s' for message: '%s'.\n", _fileName.c_str(),
 in_message.c_str() );
   }
   else
   {
     sout << in_message << endl;
   }
   return sout.good();
}

Notice that the usage of fail() instead of good() has the advantage that
moves the error handling about opening the file nearer to the opening
statement. I find this a plus, but your opinion may be different.
Alternatively there the even more terse style:

bool FileMessageQueue::Enqueue( const string& in_message )
{
   I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );
   ofstream sout( _fileName.c_str(), ios::app );
   if ( sout ) // implictly check for !fail()
   {
     sout << in_message << endl;
   }
   else
   {
      I_LOG( LM_ERROR, "FileMessageQueue - failed opening output stream
 for enqueue on file: '%s' for message: '%s'.\n", _fileName.c_str(),
 in_message.c_str() );
   }
   return sout.good();
}

In any case, I would avoid the use of the idiom "true == ..." to check a
boolean. It really makes the code heavier to read with no advantage
whatsoever, except maybe to comply with some rigid corporate style guides.

HTH,

Ganesh

      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

kavi81@lycos.com wrote:
> bool FileMessageQueue::Enqueue( const string& in_message )
> {
>    I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );
>    bool out = false;
>    ofstream sout( _fileName.c_str(), ios::app | ios::ate );
>    if ( true == sout.good() )
>    {
>       sout << in_message << endl;
>    }
>    else
>    {
>       I_LOG( LM_ERROR, "FileMessageQueue - failed opening output stream"
>           "for enqueue on file: '%s' for message: '%s'.\n",
>           _fileName.c_str(), in_message.c_str() );
>    }
>    out = sout.good();
>    sout.close();
>    return out;
> }
> How can I make this enqueue to write to disk(synchronise it)?

As soon as the fstream goes out of scope, it flushes all its buffers. The
data is then completely under the control of the OS, so there is nothing
C++ can do about it, you will have to use some OS-specific methods.

> Any suggestions would be very helpful.

Depending on what you are trying to achieve, there are other syncronisation
measures available, but in order to give any advise there you first need to
tell us what you want.
Other than that, I noticed that 
 - The receiving end might be confused when the inputvalue contained a
newline.
 - It won't be logged when the write operation fails, only when opening the
stream failed.
 - You call sout.good() redundantly when opening failed.
 - There is no need to call close() before leaving the function.
 - You should call flush() to make sure that all data was written and only
then return the value of 'sout.good()'. Else, the actual data might still
be buffered and lateron the writing fails unnoticed.
 - Consider using exceptions to signal errors. These can't be ignored like
returnvalues and you can separate the error-handling from the actual code.

Uli


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Ulrich Eckhardt wrote:
> kavi81@lycos.com wrote:
>>    if ( true == sout.good() )
>>    {
>>       sout << in_message << endl;
>>    }
[...]
>  - You should call flush() to make sure that all data was written and only
> then return the value of 'sout.good()'. Else, the actual data might still
> be buffered and lateron the writing fails unnoticed.

Just something which only occured to me after reading Alberto's answer to
your posting: 'std::endl' actually writes a newline and then flushes the
stream, so part of my point is moot. It's only that it's more explicit.

sorry

Uli


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

kavi81@lycos.com wrote:

>    I have  a c++ code which is a message queue.

>  Here is the code for enqueue:

> ##########################################################
> Enqueue
> ##########################################################
> bool FileMessageQueue::Enqueue( const string& in_message )
> {
>    I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );
>    bool out = false;
>    ofstream sout( _fileName.c_str(), ios::app | ios::ate );
>    if ( true == sout.good() )

    if ( sout )

simply?

I'm not sure what sout.good() means in this case; I think that
if the file is empty, it is allowed to be false.  At any rate,
I've yet to see correct code which uses it.

Also, the reason you compare with something in a conditional is
to get a result of type bool.  If you've already got that, you
don't compare.

>    {
>       sout << in_message << endl;
>    }
>    else
>    {
>       I_LOG( LM_ERROR, "FileMessageQueue - failed opening output stream
> for enqueue on file: '%s' for message: '%s'.\n", _fileName.c_str(),
> in_message.c_str() );
>    }
>    out = sout.good();

Ditto here.  Also, it's useless to check the state before the
close.

>    sout.close();
>    return out;

    return sout ;

Is all you need, since you're not doing any of the error
handling here, but propgating it by return code.

> }
>
>   #######################################################

> How can I make this enqueue to write to disk(synchronise it)?

You can't, at least not in pure C++.

My usual solution is to use the Unix interface; if formatting is
needed, I format into an ostringstream, and output the resulting
string.  In this case, an ostringstream would probably be
overkill, and I'd do something like:

    int fd = open( myFilename.c_str(),
                   O_WRONLY | O_APPEND | O_CREAT | O_SYNC ) ;
    bool                written = false ;
    if ( fd < 0 ) {
        //  error, couldn't open...
    } else {
        iovec               buffer ] =
        {
            { in_message.data(), in_message.size() },
            { "\n"             , 1                 },
        } ;
        if ( writev( fd, &buffer, 2 ) == in_message.size() + 1 ) {
            written = true ;
        }
        close( fd ) ;
    }
    return written ;

(I'd verify that writev is atomic with O_APPEND in your
environment.  My interpretation of the request in the
specification, it should be, but it's not as clear as it should
be.  Of course, if yours is the only process writing to the
file, this shouldn't be a problem.)

But of course, this supposes a Posix API.  I'm sure that there
is something similar under Windows, but I'm not familiar with
it.

--
James Kanze                                           GABI Software
Conseils en informatique orient�e objet/
                   Beratung in objektorientierter Datenverarbeitung
9 place S�mard, 78210 St.-Cyr-l'�cole, France, +33 (0)1 30 23 00 34


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Alberto Ganesh Barbati wrote:
> kavi81@lycos.com wrote:

> > How can I make this enqueue to write to disk(synchronise
> > it)?  Any suggestions would be very helpful.

> Please explain what do you need with "write to
> disk(synchronise it)".

The usual meaning is that the data is physically written to the
medium before the function returns.  As you point out later,
there is no way in C++ to ensure this.

> Really that's obscure to me. Write what? Syncronise what? The
> code *is* indeed writing something to disk...

No.  The code is issuing a request that something be written.
There's a big difference.

> so what are you trying to achieve, exactly?  If you mean
> "flushing" the stream buffer, then closing the stream should
> already be enough.

If 1) the system does no buffering, and 2) the close does not
cause an error.

> If you mean physically writing to disk (in case there's a disk
> cache, for example) that's an O/S detail and it's not covered
> by the C++ language; there's no portable way to achieve that.

It's never the less very often necessary.  In practice, it means
that about the only thing I use fstream for is logging.  (Even
then, since I'm usually dealing with filtering streambuf's and
ostream wrappers there, I'll generally only use a filebuf, and
not an ofstream.)

> Anyway, I can give you a few style advices regarding iostream
> usage.  It's my personal style so take them for what's worth:

> 1) avoid using ios::ate: although it's standard, it's
> implemented differently by different O/S. In your case
> ios::app already provides what you need (appending at the end
> of a text file), so you don't need ios::ate anyway.

Agreed.  All ate does (or should do) is execute a seek to the
end of the file after the open.  Which is pretty useless.  The
motivation for app is that the seek to the end and the write
should be atomic, if the system supports it.

> 2) usually you don't use good() to check the stream status,

Not: usually..don't.  Never.  I've yet to see a correct program
which uses good().

> but use fail() instead. The first will return false on eof,
> and eof is usually *not* a failure. As in your case it's an
> output stream, it doesn't really make a difference,

I'm not sure of that.  I think that if the file is empty, or if
you are positionned at the end of it, the system can set eof.
But I'm not really sure; it's not that clear what eof means on
an output file.  (Actually, the exact wording in the standard is
that it is true if "an input operation reached the end of an
input sequence".  Still, I've heard rumors that certain
implementations do set it if you open an empty file.  Best to
avoid it, just in case.)

> but for an istream it might. I usually prefer to be consistent
> between the input and output case.

Agreed.

The available functions, fail(), good() and bad() are confusing.
The more or less standard idiom is to use the conversion
operator or !, e.g. to write "if ( stream )", or "if ( ! stream)"
and be done with it.  (Once an input stream has failed, it may
be useful to check eof() or bad() to know why.  But before the
stream fails, I never use any of these functions.)

> 3) the stream destructor will automatically close the stream,
> so you don't need to call close() explictly.

Except that you need the state of the stream after the close.
It is almost always an error to depend on the destructor to call
close in an output stream.  Since he's concerned with
synchronization, it is certainly an error.

> For example, if I were to write your code, I would write it
> like this:

> bool FileMessageQueue::Enqueue( const string& in_message )
> {
>    I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );
>    ofstream sout( _fileName.c_str(), ios::app );
>    if ( sout.fail()  )

    if ( ! sout ) ...

Same thing, really, but more idiomatic.

>    {
>       I_LOG( LM_ERROR, "FileMessageQueue - failed opening output stream
>  for enqueue on file: '%s' for message: '%s'.\n", _fileName.c_str(),
>  in_message.c_str() );
>    }
>    else
>    {
>      sout << in_message << endl;
>    }
>    return sout.good();

And you've just made the same mistake he did.  Used good().

In fact, of course, you've probably tested nothing; all that has
happened so far is that the stream has copied your message into
its internal buffer.

Better would be:

    sout.close() ;
    return sout ;

But even here, all that you are testing is that the data has
been transfered to the OS.  Whether this is sufficient depends
enormously on the application, but since he said he needed the
data synchronized, it's probable that it isn't enough; that if
the function returns true, the calling code will send out the
acknowledgements, etc., that the transaction has successfully
finished.  When this isn't the case.

> }

> Notice that the usage of fail() instead of good() has the
> advantage that moves the error handling about opening the file
> nearer to the opening statement. I find this a plus, but your
> opinion may be different.

I tend to agree.  On the other hand, if the error handling is
really long, and the correct case very short, I will invert
them; I like to keep the else as close to the if as possible as
well.  (On the third hand, of course, if this is the case, I'll
probably split the error handling off into a separate function,
so it becomes a single function call here.)

> Alternatively there the even more
> terse style:

> bool FileMessageQueue::Enqueue( const string& in_message )
> {
>    I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );
>    ofstream sout( _fileName.c_str(), ios::app );
>    if ( sout ) // implictly check for !fail()
>    {
>      sout << in_message << endl;
>    }
>    else
>    {
>       I_LOG( LM_ERROR, "FileMessageQueue - failed opening output stream
>  for enqueue on file: '%s' for message: '%s'.\n", _fileName.c_str(),
>  in_message.c_str() );
>    }
>    return sout.good();

Again:

    sout.close() ;
    return sout ;

> }

> In any case, I would avoid the use of the idiom "true == ..."
> to check a boolean. It really makes the code heavier to read
> with no advantage whatsoever, except maybe to comply with some
> rigid corporate style guides.

Generally, output errors are exceptional enough that you would
use exceptions for them.  Except in the critical case where you
need to know immediately (in the calling function) whether the
output succeeded or not, so that you can determine whether to
send a positive or negative acknowledge.  Which is typically the
case with synchronized writes.  Not knowing more about his
application, I'd hesitate to say which is more appropriate here.

--
James Kanze                                           GABI Software
Conseils en informatique orient�e objet/
                   Beratung in objektorientierter Datenverarbeitung
9 place S�mard, 78210 St.-Cyr-l'�cole, France, +33 (0)1 30 23 00 34


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

kanze wrote:
> Alberto Ganesh Barbati wrote:
>>kavi81@lycos.com wrote:
>>2) usually you don't use good() to check the stream status,
>
> Not: usually..don't.  Never.  I've yet to see a correct program
> which uses good().
>
>>but use fail() instead. The first will return false on eof,
>>and eof is usually *not* a failure. As in your case it's an
>>output stream, it doesn't really make a difference,
>
> I'm not sure of that.  I think that if the file is empty, or if
> you are positionned at the end of it, the system can set eof.
> But I'm not really sure; it's not that clear what eof means on
> an output file.  (Actually, the exact wording in the standard is
> that it is true if "an input operation reached the end of an
> input sequence".  Still, I've heard rumors that certain
> implementations do set it if you open an empty file.  Best to
> avoid it, just in case.)

Doh! In fact I was relying on the fact that every reference in the
standard to eofbit is about something that happens on the input
sequence. So I assumed that eofbit would never be set for ofstreams,
which do not have an input sequence. Are you saying that my assumption
is wrong or that there are implementation out there that are
non-conformant on this point? Anyway, I already agreed that checking
fail() (or using the idiomatic "if(s)" and "if(!s)" is better style ;-)

>>3) the stream destructor will automatically close the stream,
>>so you don't need to call close() explictly.
>
> Except that you need the state of the stream after the close.
> It is almost always an error to depend on the destructor to call
> close in an output stream.  Since he's concerned with
> synchronization, it is certainly an error.

Correct. I have missed this. Thanks for pointing that out.

> Generally, output errors are exceptional enough that you would
> use exceptions for them.  Except in the critical case where you
> need to know immediately (in the calling function) whether the
> output succeeded or not, so that you can determine whether to
> send a positive or negative acknowledge.  Which is typically the
> case with synchronized writes.  Not knowing more about his
> application, I'd hesitate to say which is more appropriate here.

Agreed. I also would use exceptions in this case, unless there's a very
good reason not to use them. I would still handle the "open failed" case
separately, in order to provide better diagnostic. Something like this:

void FileMessageQueue::Enqueue( const string& in_message )
{
    I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );
    ofstream sout( _fileName.c_str(), ios::app );
    if ( !sout )
    {
       // format error message in msg omitted for brevity
       throw std::runtime_error(msg);
    }
    sout.exceptions(ios_base::failbit | ios_base::badbit);
    sout << in_message << endl;
}

Of course the function does no longer need to return a bool.

Ganesh

      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

kavi81@lycos.com wrote:
> Hi,
>    I have  a c++ code which is a message queue.
>
>
>  Here is the code for enqueue:
>
>
> ##########################################################
> Enqueue
> ##########################################################
> bool FileMessageQueue::Enqueue( const string& in_message )
> {
>    I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );
>
>
>
>    bool out = false;
>
>
>    ofstream sout( _fileName.c_str(), ios::app | ios::ate );
>
>
>    if ( true == sout.good() )
>    {
>       sout << in_message << endl;
>    }
>    else
>    {
>       I_LOG( LM_ERROR, "FileMessageQueue - failed opening output stream
>
> for enqueue on file: '%s' for message: '%s'.\n", _fileName.c_str(),
> in_message.c_str() );
>    }
>
>
>    out = sout.good();
>
>
>    sout.close();
>
>
>    return out;
>
>
>
> }
>
>
>   #######################################################
> How can I make this enqueue to write to disk(synchronise it)?
> Any suggestions would be very helpful.

Let's assume that your _fileName.c_str() returns the name of
existing FIFO (named pipe). It would be interresting what effects this
code
produce on this. When you correct code as others sugested,
FIFO would take care o buffering as in this case nothing is written
on a disk. You just have to be prepared to handle SIGPIPE
if other end closes a pipe before ofstream writes all data.

Greetings, Bane.


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

Alberto Ganesh Barbati wrote:
> kanze wrote:
> > Alberto Ganesh Barbati wrote:
> >>kavi81@lycos.com wrote:
> >>2) usually you don't use good() to check the stream status,

> > Not: usually..don't.  Never.  I've yet to see a correct program
> > which uses good().

> >>but use fail() instead. The first will return false on eof,
> >>and eof is usually *not* a failure. As in your case it's an
> >>output stream, it doesn't really make a difference,

> > I'm not sure of that.  I think that if the file is empty, or
> > if you are positionned at the end of it, the system can set
> > eof.  But I'm not really sure; it's not that clear what eof
> > means on an output file.  (Actually, the exact wording in
> > the standard is that it is true if "an input operation
> > reached the end of an input sequence".  Still, I've heard
> > rumors that certain implementations do set it if you open an
> > empty file.  Best to avoid it, just in case.)

> Doh! In fact I was relying on the fact that every reference in
> the standard to eofbit is about something that happens on the
> input sequence. So I assumed that eofbit would never be set
> for ofstreams, which do not have an input sequence. Are you
> saying that my assumption is wrong or that there are
> implementation out there that are non-conformant on this
> point?

As I said, I'm not sure.  I do seem to remember hearing about an
implementation which set eofbit when opening an empty file, but
I don't know whether this was systematic, or only for input
files.

As a general, rule, ios::good() is a function to avoid.

     [...]
> > Generally, output errors are exceptional enough that you
> > would use exceptions for them.  Except in the critical case
> > where you need to know immediately (in the calling function)
> > whether the output succeeded or not, so that you can
> > determine whether to send a positive or negative
> > acknowledge.  Which is typically the case with synchronized
> > writes.  Not knowing more about his application, I'd
> > hesitate to say which is more appropriate here.

> Agreed. I also would use exceptions in this case, unless
> there's a very good reason not to use them.

As I said, I'm not sure.  When in doubt, I use a return code;
it's much easier for the caller to remap a return code into an
exception than the reverse.

In this case, of course, I don't think it's some general purpose
function, but rather an intrinsic part of the application.  In
that case, you should know more or less where the error is to be
handled, and be able to make the correct choice without
problems.

> I would still handle the "open failed" case separately, in
> order to provide better diagnostic.

Agreed.

> Something like this:

> void FileMessageQueue::Enqueue( const string& in_message )
> {
>     I_TRACE( LVL_NORMAL, "FileMessageQueue::Enqueue( const string& )" );
>     ofstream sout( _fileName.c_str(), ios::app );
>     if ( !sout )
>     {
>        // format error message in msg omitted for brevity
>        throw std::runtime_error(msg);
>     }
>     sout.exceptions(ios_base::failbit | ios_base::badbit);
>     sout << in_message << endl;

I don't think so.  Why do you want a special, formatted message
for the first case, and not for this case?  I'd still do:

     sout << in_message << std::endl ;
     sout.close() ;
     if ( ! sout ) {
         //  format message...
         throw ...
     }

> }

--
James Kanze                                           GABI Software
Conseils en informatique orient�e objet/
                    Beratung in objektorientierter Datenverarbeitung
9 place S�mard, 78210 St.-Cyr-l'�cole, France, +33 (0)1 30 23 00 34


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]