patchPSPP - Patches: patch #5907, Implement ADD DOCUMENT command.

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #5907: Implement ADD DOCUMENT command.

Submitter:  John Darrington <jmd>
Submitted:  Wed 02 May 2007 10:34:10 AM UTC
   
 
Category:  None Item Group:  None
Status:  Done Assigned to:  None
Open/Closed:  Closed

Thu 03 May 2007 02:31:46 AM UTC, comment #5: 


>But I'm not happy about the way the document semantics are
>scattered all over the code, so expect another patch from me
>shortly.


Yes, I'd also prefer that it be better abstracted.

Ben Pfaff <blp>
Group administrator
Thu 03 May 2007 02:20:58 AM UTC, comment #4: 

I'll do that as a stop-gap measure so we can get this patch in.

But I'm not happy about the way the document semantics are scattered all over the code, so expect another patch from  me shortly.

John Darrington <jmd>
Group administrator
Thu 03 May 2007 01:49:20 AM UTC, comment #3: 

How about just making dict_set_documents enforce a document string being an exact multiple of 80 bytes in length, e.g.


--- dictionary.c.~1.32.2.2.~        2007-04-22 12:01:47.000000000 -0700
+++ dictionary.c        2007-05-02 18:48:57.000000000 -0700
@@ -1104,6 +1104,7 @@ void
 dict_set_documents (struct dictionary *d, const char *documents)
 {
   assert (d != NULL);
+  assert (documents == NULL || strlen (documents) % 80 == 0);

   free (d->documents);
   if (documents == NULL)


Ben Pfaff <blp>
Group administrator
Thu 03 May 2007 12:15:35 AM UTC, comment #2: 


> In the documentation, I don't think that the . should be on a separate line; although that's valid syntax, it's not a style I've commonly seen.


OK.


> In cmd_add_documents, please add a period to the end of the error message.


OK.

> I don't think the "x / 80" => "DIV_RND_UP (x, 80)" changes are correct. In each case, if the number of bytes is not a multiple of 80, then rounding up will cause subsequent code to read beyond the end of the document string.


Consider what happens if x < 80.  In that case x / 80 is zero.  So n_lines will get the value zero, and when reading the file back, the error on sys-file-reader.c:671 will occur (as I found out when experimenting).  In general, I think the n_lines value which gets written by write_documents (in sys-file-writer.c) should be the number of complete or partial lines.  So I think DIV_RND_UP is correct. Otherwise the last line will get dropped unless it happens to be exactly 80 bytes long.

Currently, of course, everything is written through add_document_line which ensures that the 2nd argument to dict_set_documents is always a multiple of 80 bytes.  However, whilst we're using / and not DIV_RND_UP, dict_set_documents is unsafe.

To prevent reading beyond the end of the document string,  I suggest that we pad it to an 80 byte multiple before calling buf_write.

John Darrington <jmd>
Group administrator
Wed 02 May 2007 02:38:22 PM UTC, comment #1: 

Some quibbles:

In the documentation, I don't think that the . should be on a separate line; although that's valid syntax, it's not a style I've commonly seen.

In cmd_add_documents, please add a period to the end of the error message.

I don't think the "x / 80" => "DIV_RND_UP (x, 80)" changes are correct.  In each case, if the number of bytes is not a multiple of 80, then rounding up will cause subsequent code to read beyond the end of the document string.

Ben Pfaff <blp>
Group administrator
Wed 02 May 2007 10:34:10 AM UTC, original submission:  

This patch implements the ADD DOCUMENT command.
It's similar to the DOCUMENT command, but takes quoted strings instead of literal text.

John Darrington <jmd>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #12662:  ad.patch added by jmd (15KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by blp (Posted a comment)
  • -email is unavailable- added by jmd (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-05 jmd StatusWorks For Me Done
        Assigned toblp None
        Open/ClosedOpen Closed
    2007-05-03 jmd Assigned toNone blp
    2007-05-02 blp StatusReady For Test/Review Works For Me
    2007-05-02 jmd Attached File- Added ad.patch, #12662

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code