From: Andy C. <an...@ad...> - 2001-04-28 12:36:37
|
Last year I had trouble getting isc_expand_dpb() to work. It turns out = that it CAN'T work as described in the manual. The problems are in the = interface, not in the code. What's wrong with isc_expand_dpb() -=20 [1] There is a difference between the size of a buffer and the size of = the data in the buffer. The API says that isc_expand_dpb() allocates a = new buffer if the new material will not fit into the old buffer. = However, the length parameter is the size of the data in the buffer; = isc_expand_dbp() has no way of knowing how big the old buffer is.=20 [2] The API says that isc_expand_dpb() allocates a new buffer if it = needs it. However, there is nothing said about what happens to the old = buffer. There is no deallocation of the old buffer. Because these flaws are in the functional specs of isc_expand_dpb(), and = not in the code itself, I propose to repair them by providing two new = functions: isc_fill_dpb() takes exactly four parameters: A pointer to a DBP. Before the first call to isc_fill_dbp(), the = caller should set this pointer to NULL. isc_fill_dbp() will do all = allocating of the dbp buffer. A pointer to a short int giving the length of the data in the = dbp_buffer. This is purely an output of isc_fill_dpb(), and is provided = solely for compatablity with isc_attach_database(), which needs this = number as a parameter. A parameter code for the string that is to be inserted into the dbp. A pointer to the string that is to be inserted into the dbp. Note that isc_fill_dbp() does not take variable arguments, like = isc_expand_dpb(). Variable arguments must bypass compile-time type = checking, and thus are error-prone. isc_free_dpb() takes exactly one parameter, the pointer to the DPB as = created and filled by isc_fill_dbp(). This must be called AFTER the call = to isc_attach_database, to release the memory in the buffer. Code calling these function would look like this: char * username =3D ...; char * password =3D ...; char * dpb =3D NULL; short dbp_length; isc_fill_dpb( & dpb, & dpb_length, isc_dpb_user_name, username ); isc_fill_dpb( & dpb, & dpb_length, isc_dpb_password, password ); isc_attach_database( & status_vector, strlen( dbname ), dbname,=20 & dbhandle, dpb_length, dbp ); isc_free_dpb( dbp ); To implement this we would need a new dpb code which the caller never = sees and which marks the allocated length of the dpb and the actual data = length of the dbp (two numbers). Because shorts look a lot smaller now = than they did a few years ago, I'd like to make these four byte numbers. = Therefore the length information block would be ten bytes: one byte for isc_dpb_length one byte with the value 9 a four byte unsigned integer, the allocated length, standard (VAX?) = format a four byte unsigned integer, the data length, in standard (VAX?) = format I believe that this implementation meets the following criteria: * It constructs a dbp which, with the exception of the new = isc_dpb_length field, is completely compatable with the existing dpb = format. I am led to understand that isc_attach_database() ignores any = dpb code that it doesn't understand; therefore it will ignore the = isc_dpb_length code and work as before. * It can be fully thread-friendly. The application can construct two = dbp's at the same time, interleaving the calls to isc_fill_dbp() at = will, with no clash of buffer. * There is no memory leak (unless the caller forgets the call to = isc_free_dpb()). No matter how many times the caller calls = isc_free_dbp(), no matter how many times isc_free_dbp() must re-allocate = the dbp buffer; because isc_free_dpb() knows how the buffer was = allocated, it will free it whenever it must replace it. * with fixed parameters to isc_free_dpb(), not variable parameters, we = have type checking on the code. * When we wish to, we can cause isc_attach_database to store the = dpb_length in an unsigned long integer, and recognize the isc_dpb_length = code, updating the (small) number it received from the caller with the = (large) number it finds in the dbp itself. When we do this we will no = longer be limited to 32767 bytes in the dbp.=20 I am a little concerned that the length of a string in the dbp buffer is = limited to 255 characters by the use of one byte as a length byte; file = names under Windows 98 are limited to 32,767 bytes. If the user calls = isc_fill_dpb() to put the strings into the dbp, we can set the format = code to "2" and make the upgrade invisible to the caller. Of course we need to keep isc_expand_dpb() in the API to support old = code; however we should discourage it's use for new code and shift over = to the new functions. Users would be warned that they can use = isc_fill_dbp() and isc_free_dpb(), or they can do it themselves, but = they should not mix methods as isc_fill_dpb() is not guaranteed to = follow the format given in the API. I welcome any comments. According to the API, isc_expand_dpb() and = isc_attach_database() are the only two functions that use the dbp at = all. But there is a TPB - maybe we should make the code handle both? How = does this fix fit in with plans that others have? |
From: Ann W. H. <aha...@ib...> - 2001-04-28 17:39:00
|
Andy, May I suggest putting this on the architecture list or the firebird development list? Or maybe not after you've read my comments? >What's wrong with isc_expand_dpb() - > >[1] There is a difference between the size of a buffer and the size of the >data in the buffer.... That's an error in the example - the caller should supply the length of the buffer, not the length of the data. >[2] The API says that isc_expand_dpb() allocates a new buffer if it needs >it. However, there is nothing said about what happens to the old buffer. >There is no deallocation of the old buffer. The comments in the code say that the original intended user for this (gpre) generates code to release the allocated buffer. Gpre also generates exactly one call to isc_expand_dpb, so there are no intermediate allocations to get lost. FYI, dpb stands for database parameter block. >... isc_fill_dpb( & dpb, & dpb_length, isc_dpb_user_name, username ); >... isc_free_dpb( dbp ); without adding two more calls to the (bloated) interface, you could just allocated and stuff the block yourself. It's simple. one byte of dpb version, the identifier, a byte of length, then the value, and continue with identifier, length, value triplets until done. Keep track of the length and if your next value will overflow, allocate a bigger block, copy the data, & release the old block. >* When we wish to, we can cause isc_attach_database to store the >dpb_length in an unsigned long integer, and recognize the isc_dpb_length >code, updating the (small) number it received from the caller with the >(large) number it finds in the dbp itself. When we do this we will no >longer be limited to 32767 bytes in the dbp. That limit has not yet been a problem. >I am a little concerned that the length of a string in the dbp buffer is >limited to 255 characters by the use of one byte as a length byte; file >names under Windows 98 are limited to 32,767 bytes. Again, that limit hasn't been a problem up to now - very few people actually use names longer than 200 characters because of the improbability of typing them right. >If the user calls isc_fill_dpb() to put the strings into the dbp, we can >set the format code to "2" and make the upgrade invisible to the caller. I don't see this as worth the overall cost. >Of course we need to keep isc_expand_dpb() in the API to support old code; >however we should discourage it's use. I'd just stop there. >I welcome any comments. According to the API, isc_expand_dpb() and >isc_attach_database() are the only two functions that use the dbp at all. >But there is a TPB - maybe we should make the code handle both? How does >this fix fit in with plans that others have? The tpb (transaction parameter block) works like the dpb, and like creating the dbp, creating a tpb is within the skill set of any dull-normal programmer. Regards, Ann www.ibphoenix.com We have answers. |