'RE: [Common-dev] CR: Implementation for Simple File reader to read'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       helix-common-dev
Subject:    RE: [Common-dev] CR: Implementation for Simple File reader to read
From:       "Eric Hyche" <ehyche () real ! com>
Date:       2009-02-24 16:57:06
Message-ID: 007f01c996a0$ec903fa0$c5b0bee0$ () com
[Download RAW message or body]

Also, VS8 told me that the .cpp file has inconsistent line
endings. Please fix this before checking in.

Eric

=======================================
Eric Hyche (ehyche@real.com)
Principal Engineer
RealNetworks, Inc.

>-----Original Message-----
>From: Gaurav Bajaj [mailto:gbajaj@real.com]
>Sent: Monday, February 23, 2009 2:52 AM
>To: ehyche@real.com
>Cc: common-dev@helixcommunity.org
>Subject: Re: [Common-dev] CR: Implementation for Simple File reader to read small files like nsc files
>
>Thanks Eric,
>
>Please find the files att. incorporating your suggestions.
>
>-Gaurav
>
>
>Eric Hyche wrote:
>
>	Gaurav,
>
>	Here are my comments:
>
>	1) Both new files need license headers. Use
>	   the license header in section 2.1.1 of
>	   http://asg-plone.dev.prognet.com/helix/moin.cgi/SourceFileHeaders
>
>	2) In hxsimplefilereader.h -
>
>	   CHXSimpleFileReaderResponse should be an abstract class
>	   instead of a concrete one. So you can have "PURE" after
>	   all of the IUnknown methods, and there's no need for
>	   a m_lRefCount member variable.
>
>	3) I noticed everything in hxsimplefilereader.h is
>	   offset 3 spaces from the left. Everything should
>	   start in column 0.
>
>	4) hxsimplefilereader.h - HX_RESULT   GetFile(IHXFileObject* /*IN*/ pFile);
>
>	  I would call this GetFileObject() instead.
>
>	5) hxsimplefilereader.cpp - WriteDone() should return HXR_NOTIMPL,
>	   not HXR_OK. HXR_OK implies the write succeeded.
>
>	6) 	rpReader = new CHXSimpleFileReader(pContext);
>		rpReader->AddRef();
>		return HXR_OK;
>
>	   NEVER assume that a new or malloc succeed and use
>	   the pointer without first checking the pointer for
>	   non-NULL. And if it doesn't succeed, you should
>	   return HXR_OUTOFMEMORY.
>
>	7) ReadFile() should fail with HXR_INVALID_PARAMETER
>	   if either pResponse or pFileObject are NULL.
>
>	8) In ReadFile() you need to set         m_bGetFilePending = TRUE;
>	   BEFORE you call m_pFile->Init().
>
>	9)        , m_pContext (NULL)
>	{
>	    if (!m_pContext)
>	    {
>	        m_pContext = pContext;
>	        HX_ADDREF(m_pContext);
>	    }
>
>	   Not sure why you need the if (!m_pContext) check.
>	   You know it's going to be NULL.
>
>	10) m_pBuffer needs to get HX_RELEASE()'d in Close().
>
>	11) You are never calling Close() on m_pFile. The
>	    file object must be closed - it should be closed
>	    when the file object reader gets a Close().
>
>	12) You are never HX_RELEASE()'ing m_pReaderResponse.
>
>	13) In InitDone() you need to Seek() the file object
>	   to 0. You don't know whether or not some other
>	   object has already done some operations on that
>	   file object before you get it, and you cannot
>	   assume that Init() sets the file cursor back to 0.
>
>	14) 	ret = m_pFile->Read(4096);//FILEREAD_SIZE);
>
>	   You read 4096 bytes in multiple places. 4096 should
>	   be made into a preprocessor define at the top of the .cpp
>	   file, so that if you want to change the read size, you
>	   only have to change it in one place.
>
>	15) 	CreateBufferCCF(pMergedBuffer, m_pContext);
>	        if (!pMergedBuffer)
>	        {
>	            ret = HXR_OUTOFMEMORY;
>	            goto cleanup;
>	        }
>
>	  If this happens, you will never return ReadFileDone()
>	   to the response and the application will hang.
>
>	16)            pMergedBuffer->SetSize(pBuffer->GetSize());
>	            pMergedBuffer->Set(pBuffer->GetBuffer(), pBuffer->GetSize());
>
>	   Two problems here:
>	      a) there is no need for both a SetSize() and a Set() call. Just
>	         a Set() will suffice;
>	      b) you are not checking the return value. Set() and SetSize()
>	         allocate memory, which could fail. You must check the return
>	         value and act on it.
>
>	    Same thing in the else{} clause.
>
>	17) You might also want to consider using an IHXFragmentedBuffer
>	    instead of this allocating a new buffer every time. An IHXFragmentedBuffer
>	    is an object that you can add multiple IHXBuffer's to in sequence, and
>	    then it will present one IHXBuffer interface that spans all of
>	    the buffers. It minimizes the realloc's and the memcpy's.
>
>	18)            // read error
>	            m_pFile->Seek(0, FALSE);
>
>	   I don't think there's any need to do this.
>
>	19) CHXSimpleFileReader has a Close() method defined in the .h
>	    file but it is not implemented. It definitely needs a Close() method
>	    so that a user can immediately shut down the reading if necessary.
>	    Also, without a Close() method, we will have a leak since the user
>	    holds a ref on the object and the object probably holds a ref
>	    on the user.
>
>	    It should probably look like:
>
>	    HX_RESULT CHXSimpleFileReader::Close()
>	    {
>	        if (m_pFile)
>	        {
>	            m_pFile->Close();
>	        }
>	        HX_RELEASE(m_pBuffer);
>	        HX_RELEASE(m_pFile);
>	        HX_RELEASE(m_pContext);
>	        HX_RELEASE(m_pReaderResponse);
>	    }
>
>	   and then in the destructor, you can just call Close(), like this:
>
>	   CHXSimpleFileReader::~CHXSimpleFileReader()
>	   {
>	       Close();
>	   }
>
>
>	=======================================
>	Eric Hyche (ehyche@real.com)
>	Principal Engineer
>	RealNetworks, Inc.
>
>
>
>
>		-----Original Message-----
>		From: common-dev-bounces@helixcommunity.org [mailto:common-dev-bounces@helixcommunity.org]
>On Behalf
>		Of Gaurav Bajaj
>		Sent: Friday, February 20, 2009 6:32 AM
>		To: common-dev@helixcommunity.org
>		Subject: [Common-dev] CR: Implementation for Simple File reader to read small files like
>nsc files
>
>		Synopsis:
>
>
>		Adding support for file reader to read simple and small files like .nsc.
>
>		Overview:
>		These files are needed to read nsc file in one go, which later, can be parsed and used to
>initiate MSB
>		Client to receive MSB Packet from the server whose Ip address has been mentioned in the
>.nsc file
>
>
>
>		Files Modified:
>		common\fileio\umakefil
>
>
>		Files Added:
>		common\fileio\hxsimplefilereader.cpp
>		common\fileio\hxsimplefilereader.h
>
>
>
>		Image Size and Heap Use impact (Client -Only):
>		None.
>
>		Platforms and Profiles Affected:
>		None
>
>		Distribution Libraries Affected:
>		None.
>
>		Distribution library impact and planned action:
>		None.
>
>		Platforms and Profiles Build Verified:
>		    Profile: helix-client-all-defines
>
>		branch:  atlas 310
>		  Target: splay
>
>		Branch: atlas 310
>
>		Files att
>		hxsimplefilereader.cpp
>		hxsimplefilereder.h
>		diff.txt
>
>		Thanks & Regards
>		Gaurav Bajaj
>
>
>
>
>
>

_______________________________________________
Common-dev mailing list
Common-dev@helixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/common-dev
[prev in list] [next in list] [prev in thread] [next in thread] 