[poppler] Patch to Get/Set PDF ID in the trailer dictionary

[Carlos Garcia Campos]

Excerpts from Pino Toscano's message of mié jun 16 15:45:49 +0200 2010:
> Hi,
> 
> Alle mercoledì 16 giugno 2010, srinivas adicherla ha scritto:
> > Index: poppler/poppler-poppler-document.html
> > Index: poppler/poppler-poppler.html
> > [...]
> >            Generated by GTK-Doc V1.11</div>
> 
> The HTML files are autogenerated, so pretty unuseful. I'm not familiar 
> with gtk-doc, so I cannot tell you what to do (Carlos?).

Documentation should be added as comments in source code, HTML is
generated by gtk-doc. 

> > +  // Return the PDF ID contains in the trailer dictionary 
> > +  GBool getID(GooString *permanent_id,GooString *update_id);
> > +  // Set the PDF ID in the trailer dictionary
> > +  GBool   setID();
> 
> A bit more of clean indentation: add a space after a comma, and just 
> once space instead of few. (This applies also for other parts.)
> 
> > +GBool PDFDoc::getID(GooString *permanent_id,GooString *update_id) {
> > +
> > +       Object *trailer = xref->getTrailerDict();
> 
> We don't use tabs in the poppler core, so please use the same 
> indentation used in this file.
> 
> > +       char *Id1       = val1.getString()->getCString();
> > +       char *Id2       = val2.getString()->getCString();
> > +
> > +       char **pdfids = (char**)malloc(2*sizeof(char*));
> > +       pdfids[0] = (char*)malloc(32*sizeof(char));
> > +       pdfids[1] = (char*)malloc(32*sizeof(char));
> > +
> > +       get_id(Id1,pdfids[0]);
> > +       get_id(Id2,pdfids[1]);
> > +
> > +       permanent_id->append(pdfids[0],32);
> > +       update_id->append(pdfids[1],32);
> > +       
> > +       free(Id1);
> > +       free(Id2);
> 
> Still malloc() abuse...
>   char tmpid[33];
>   get_id(val1.getString()->getCString(), tmpid);
>   permanent_id->append(tmpid, 32);
>   get_id(val2.getString()->getCString(), tmpid);
>   update_id->append(tmpid, 32);
> 
> Plus, you must free() the two Objects read.
> 
> > +GBool PDFDoc::setID() {
> > +
> > +       GooString permanent_id,update_id;
> > +       
> > +       if(getID(&permanent_id,&update_id)) { return false; }
> > +       else {
> > +               char *filename = getFileName()->getCString(); 
> 
> Still there is no handling of non-files documents, where this line with 
> crash with a NULL pointer usage.
> 
> > +               // gets filename from the path 
> > +               char *file = strrchr(filename,'/');
> > +               if(!file) strrchr(filename,'\\');
> 
> Is this supposed to handle either Unix or Windows paths?
> If so, see the SLASH define in utils/HtmlOutputDev.h for a way to just 
> use the proper character with no need to lookup twice.
> 
> > +               // file name
> > +               if(filename)
> > +                       message.append(file+1);
> > +               else
> > +                       message.append("emptyfilename.pdf");
> 
> What is this supposed to do?
> Also note strrchr() will return NULL is cannot find the character, 
> meaning opening a document like "somedocument.pdf" (ie in the current 
> directory) will make this code crash.
> Also, I think writing a new file, with a fixed name and in the same 
> directory is a really bad idea, as
> a) you cannot be sure the current directory is writeable
> b) that could be a security issue
> It could be much better to just take a OutStream* as parameter, and let 
> the caller create the actual kind of OutStream (file, buffer, etc). This 
> also mean the glib function using setID() should take a file name (ow 
> other glib IO stuff, which I do not know).
> 
> > +               FILE *f;
> > +               f = fopen(filename,"r");
> > +               fseek(f,0,SEEK_END);
> > +               fileSize = ftell(f);
> 
> You are not doing any check for all of the three calls above (especially 
> fopen), not even closing the file.
> 
> > +               fflush(stdout);
> 
> uh?
> 
> > Index: poppler-0.14.0/glib/poppler-document.cc
> > [...]
> > +  g_object_class_install_property
> > +         (G_OBJECT_CLASS (klass),
> > +          PROP_PERMANENT_ID,
> > +          g_param_spec_string ("permanent-id",
> > +                               "Permanent Id",
> > +                               "Permanent Id of the Pdf Document",
> > +                               NULL,
> > +                               G_PARAM_READABLE));
> > +
> > +  g_object_class_install_property
> > +         (G_OBJECT_CLASS (klass),
> > +          PROP_UPDATE_ID,
> > +          g_param_spec_string ("update-id",
> > +                               "Update Id",
> > +                               "Update Id of the Pdf Document",
> > +                               NULL,
> > +                               G_PARAM_READABLE));
> 
> Still, "Pdf" -> "PDF".
> 
> > +PopplerDocumentId*
> > +poppler_document_get_pdf_id (PopplerDocument *document)
> > +{
> > +
> > +       GooString permanent_id,update_id;
> > +
> > +       if(document->doc->getID(&permanent_id,&update_id)) {
> > +
> > +               PopplerDocumentId *doc_id =
> > g_new(PopplerDocumentId,1);
> > +               strcpy(doc_id->permanent_id,permanent_id.getCString(
> > ));
> > +               strcpy(doc_id->update_id,update_id.getCString());
> 
> I suppose there are better glib functions instead of strcpy().
> 

Yes, indeed. Also, I would rename the function to something like
poppler_document_get_id(), since a poppler document is always a pdf
file. Instead of returning a new allocated struct, it would be better
to return it as an out argument, so that the struct can be allocated
in the stack by the caller.

I still don't see the point of set_id(). The document id should be
changed only when the document changes, and it's already done by
saveAs().

Regards, 
-- 
Carlos Garcia Campos
PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20100617/fdb8a221/attachment.pgp>