100069 - Need infrastructure for new print dialog

Status: VERIFIED FIXED

(Core Graveyard :: Printing: Xprint, enhancement)

Description

[Roland Mainz]

Mozilla's Xprint module needs some intrastructure for the new print dialog.

Comment 1

[Roland Mainz]

Swapping QA<-->Owner

Comment 2

[Roland Mainz]

Accepting bug, setting milestone etc. etc.
Patch follows in a few hours ...

Comment 3

[Roland Mainz]

Attachment: Patch for 2001-09-14-08-trunk

Comment 4

[Roland Mainz]

Comment on attachment 49692 [details] [diff] [review]
Patch for 2001-09-14-08-trunk

Minor flaw found... new patch follows ...

Comment 5

[Roland Mainz]

Attachment: Patch for 2001-09-14-08-trunk (2nd attempt)

Comment 6

[Roland Mainz]

Requesting r=/sr=

Note that the xprintutil*.c code is a prototype for a X.org extension library,
therefore I am using |malloc| instead of the matching NSPR call...

Comment 7

[jag (Peter Annema)]

+    printf("starting search at %d\n", (int)(s-value));
+    
+    e->start = e->s = e->s = e->value = strdup(s);

Raw printf, e->s appears twice in that list

+    if (!value && value[0]!='{' && value[0]!='\0')
+      return(False);

If value is ever |0| you will segfault there.


+    /* remove '{' && '}' */
+    s = value;
+    d = name;
+    do
+    {
+      *d = tolower(*s);
+      
+      if (*s!='{' && *s!='}')
+        d++;
+      
+      s++;  
+    }
+    while(*s);

So what you want is something like:

do {
  while (*s == '{' || *s == '}')
    ++s;
  *d = tolower(*s);
  ++d;
  ++s;
} while (*s);


+    /* seperate media name from string */
+    d = (char *)search_next_space(name);
+    if (!s)

Seems like that should be |if (!d)|


I'd really, really appreciate it if someone else could also take a look at this.

Comment 8

[Roland Mainz]

jag wrote:
> +    printf("starting search at %d\n", (int)(s-value));
> +    
> +    e->start = e->s = e->s = e->value = strdup(s);
> 
> Raw printf, e->s appears twice in that list

Fixed.

> +    if (!value && value[0]!='{' && value[0]!='\0')
> +      return(False);
> 
> If value is ever |0| you will segfault there.

Ouch. Dump mistake #2.
Fixed.

> +    /* remove '{' && '}' */
> +    s = value;
> +    d = name;
> +    do
> +    {
> +      *d = tolower(*s);
> +      
> +      if (*s!='{' && *s!='}')
> +        d++;
> +      
> +      s++;  
> +    }
> +    while(*s);
> 
> So what you want is something like:
> 
> do {
>   while (*s == '{' || *s == '}')
>     ++s;
>   *d = tolower(*s);
>   ++d;
>   ++s;
> } while (*s);

Er.. no. I just want to avoid leading/training brackets. Your code does the
same... but the |while()|-loop isn't neccesary. There are usually no bracket
which follows directly another bracket.

> +    /* seperate media name from string */
> +    d = (char *)search_next_space(name);
> +    if (!s)
> 
> Seems like that should be |if (!d)|

Eeeks. dumb one, #3 ;-((
Fixed.

New patch follows...

Comment 9

[Roland Mainz]

Attachment: Patch for 2001-09-14-08-trunk

Comment 10

[Roland Mainz]

Requesting r=/sr= for the new patch ...

Comment 11

[Christopher Blizzard (:blizzard)]

Comment on attachment 49896 [details] [diff] [review]
Patch for 2001-09-14-08-trunk

>diff -r -u original/mozilla/gfx/src/xprint/xprintutil.c mozilla/gfx/src/xprint/xprintutil.c
>--- original/mozilla/gfx/src/xprint/xprintutil.c	Tue Jul 17 20:54:48 2001
>+++ mozilla/gfx/src/xprint/xprintutil.c	Wed Sep 19 16:06:45 2001
>@@ -35,6 +35,7 @@
>  
> #include <stdlib.h>
> #include <string.h>
>+#include <ctype.h>
> #include <stdio.h>
> #include <limits.h>
> #include <errno.h>
>@@ -75,7 +76,7 @@
> 
> 
> static 
>-int XpuGetPrinter2( char *printer, char *display, Display **pdpyptr, XPContext *pcontextptr )
>+int XpuGetPrinter2( const char *printer, char *display, Display **pdpyptr, XPContext *pcontextptr )
> {
>     Display   *pdpy;
>     XPContext  pcontext;
>@@ -90,13 +91,13 @@
>         int           list_count;
>         
>         /* get list of available printers... */
>-        list = XpGetPrinterList(pdpy, printer, &list_count);        
>+        list = XpGetPrinterList(pdpy, (char *)printer, &list_count);        

You just cast a |const char *| to a |char *|.  That implies that you could end up writing to read-only memory.  In fact, in the function declaration above you change the |char *| to a |const char *|.  Are you sure you want to do that?

>         if( list != NULL ) XpFreePrinterList(list);
>         
>         /* ...and check if printer exists... */
>         if( (list != NULL) && (list_count > 0) )
>         {
>-          if( (pcontext = XpCreateContext(pdpy, printer)) != (XPContext)NULL )
>+          if( (pcontext = XpCreateContext(pdpy, (char *)printer)) != (XPContext)NULL )

Same as above.

>           {
>             *pdpyptr     = pdpy;
>             *pcontextptr = pcontext;
>@@ -187,86 +188,20 @@
> void XpuSetOneAttribute( Display *pdpy, XPContext pcontext, 
>                          XPAttributes type, const char *attribute_name, const char *value, XPAttrReplacement replacement_rule )
> {
>-    char *buffer = 
>-#ifdef XPU_USE_NSPR
>-    PR_Malloc((PRUint32)
>-#else    
>-    Xmalloc(
>-#endif
>-    strlen(attribute_name)+strlen(value)+8);
>+    char *buffer = malloc(strlen(attribute_name)+strlen(value)+8);

What is the magic +8 on this malloc?  If it's required for something please put a comment above the allocation.  Magic numbers are bad.

>     
>     if( buffer != NULL )
>     {
>       sprintf(buffer, "%s: %s", attribute_name, value);      
>       XpSetAttributes(pdpy, pcontext, type, buffer, replacement_rule);
>-#ifdef XPU_USE_NSPR
>-      PR_Free
>-#else
>-      XFree
>-#endif      
>-      (buffer);
>+      free(buffer);
>     }  
> }
> 
>-
>-/* enumerate attribute values - work in porgress */
>-char *XpuEmumerateXpAttributeValue( char *value, void **context )
>-{
>-    /* BUG: This does not work for quoted attribute values
>-     * A good example is the enumeration of supported paper sizes: 
>-     * "{'' {na-letter False {6.3500 209.5500 6.3500 273.0500}} {executive False {6.3500 177.7500 6.3500 260.3500}} {na-legal False {6.3500 209.5500 6.3500 349.2500}} {iso-a4 False {6.3500 203.6500 6.3500 290.6500}} {iso-a3 False {6.3500 290.6500 6.3500 413.6500}} {iso-designated-long False {6.3500 103.6500 6.3500 213.6500}} {na-number-10-envelope False {6.3500 98.4500 6.3500 234.9500}} }"       {'' 
>-         {  
>-           na-letter 
>-           False 
>-           {6.3500 209.5500 6.3500 273.0500}
>-         } 
>-         { 
>-           executive 
>-           False 
>-           {6.3500 177.7500 6.3500 260.3500}
>-         } 
>-         {
>-           na-legal 
>-           False 
>-           {6.3500 209.5500 6.3500 349.2500}
>-         } 
>-         {
>-           iso-a4 
>-           False 
>-           {6.3500 203.6500 6.3500 290.6500}
>-         } 
>-         {
>-           iso-a3
>-           False
>-           {6.3500 290.6500 6.3500 413.6500}
>-         }
>-         {
>-           iso-designated-long 
>-           False 
>-           {6.3500 103.6500 6.3500 213.6500}
>-         } 
>-         {
>-           na-number-10-envelope 
>-           False 
>-           {6.3500 98.4500 6.3500 234.9500}
>-         } 
>-       }
>-     
>-     * ToDo: 
>-     * 1. Write a parser which grabs the 1st level of bracket pairs and 
>-     * enumerate these items (six items in this example).
>-     * 2. Write enumeration functions for single attribute types with 
>-     * complex content.
>-     */
>-    return( (char *)strtok_r(value, " ", (char **)context) );
>-}
>-
>-
> /* check if attribute value is supported or not */
> int XpuCheckSupported( Display *pdpy, XPContext pcontext, XPAttributes type, const char *attribute_name, const char *query )
> {
>-    char *value,
>-         *s;
>+    char *value;
>     void *tok_lasts;
>     
>     value = XpGetOneAttribute(pdpy, pcontext, type, (char *)attribute_name);
>@@ -275,16 +210,20 @@
>     
>     if( value != NULL )
>     {
>+      const char *s;
>+      
>       for( s = XpuEmumerateXpAttributeValue(value, &tok_lasts) ; s != NULL ; s = XpuEmumerateXpAttributeValue(NULL, &tok_lasts) )
>       {
>         XPU_DEBUG_ONLY(printf("XpuCheckSupported: probing '%s'=='%s'\n", XPU_NULLXSTR(s), XPU_NULLXSTR(query)));
>         if( !strcmp(s, query) )
>         {
>           XFree(value);
>+          XpuDisposeEmumerateXpAttributeValue(&tok_lasts);
>           return(1);
>         }  
>       }
>       
>+      XpuDisposeEmumerateXpAttributeValue(&tok_lasts);
>       XFree(value);
>     }  
>     
>@@ -442,4 +381,292 @@
>   return False;
> }
> 
>+static
>+const char *search_matching_bracket(const char *start)
>+{
>+  const char *s     = start;
>+  int         level = 0;
>+  
>+  if (!start)
>+    return(NULL);
>+    
>+  for(;;)
>+  {
>+    switch(*s++)
>+    {
>+      case '\0': return(NULL);
>+      case '{': level++; break;
>+      case '}': level--; break;
>+    }
>+    
>+    if (level == 0)
>+    {
>+      return(s);
>+    }
>+  }
>+}
>+
>+
>+static
>+const char *search_next_space(const char *start)
>+{
>+  const char *s     = start;
>+  int         level = 0;
>+  
>+  if (!start)
>+    return(NULL);
>+    
>+  for(;;)
>+  {   
>+    if(isspace(*s))
>+      return(s);
>+
>+    if(*s=='\0')
>+      return(NULL);      
>+
>+    s++;
>+  }
>+}
>+
>+/* PRIVATE context data for XpuEmumerateXpAttributeValue() */
>+typedef struct _XpuAttributeValueEnumeration
>+{
>+  char *value;
>+  char *start;
>+  char *s;
>+} XpuAttributeValueEnumeration;
>+
>+const char *XpuEmumerateXpAttributeValue( const char *value, void **vcptr )
>+{
>+    XpuAttributeValueEnumeration **cptr = (XpuAttributeValueEnumeration **)vcptr;
>+    XpuAttributeValueEnumeration  *context;
>+    
>+    if (!cptr)
>+      return(NULL);
>+      
>+    if (value)
>+    {
>+      XpuAttributeValueEnumeration *e;
>+      const char *s = value;
>+    
>+      e = malloc(sizeof(XpuAttributeValueEnumeration));
>+      if (!e)
>+        return NULL;
>+    
>+      /* skip leading '{'. */
>+      while(*s=='{')
>+        s++;
>+      /* skip leading blanks or '\''. */
>+      while(isspace(*s) || *s=='\'')
>+        s++;
>+         
>+      e->start = e->s = e->value = strdup(s);    

Are you sure that 's' has a length?  It might be a NULL or might be zero-length, right?

>+    
>+      *cptr = e;
>+    }
>+    
>+    context = *cptr;
>+    
>+    if (!context || !context->s)
>+      return(NULL);
>+     
>+    /* Skip leading blanks, '\'' or '}' */
>+    while(isspace(*(context->s)) || *(context->s)=='\'' || *(context->s)=='}' )
>+      context->s++;
>+
>+    if (*(context->s) == '\0')
>+      return(NULL);
>+
>+    context->start = context->s;
>+    if (*(context->start) == '{')
>+      context->s = (char *)search_matching_bracket(context->start);
>+    else
>+      context->s = (char *)search_next_space(context->start);
>+      
>+    /* end of string reached ? */
>+    if (context->s)
>+    {   
>+      *(context->s) = '\0';
>+      context->s%2

Comment 12

[Christopher Blizzard (:blizzard)]

Comment on attachment 49896 [details] [diff] [review]
Patch for 2001-09-14-08-trunk

>diff -r -u original/mozilla/gfx/src/xprint/xprintutil.c mozilla/gfx/src/xprint/xprintutil.c
>--- original/mozilla/gfx/src/xprint/xprintutil.c	Tue Jul 17 20:54:48 2001
>+++ mozilla/gfx/src/xprint/xprintutil.c	Wed Sep 19 16:06:45 2001
>@@ -35,6 +35,7 @@
>  
> #include <stdlib.h>
> #include <string.h>
>+#include <ctype.h>
> #include <stdio.h>
> #include <limits.h>
> #include <errno.h>
>@@ -75,7 +76,7 @@
> 
> 
> static 
>-int XpuGetPrinter2( char *printer, char *display, Display **pdpyptr, XPContext *pcontextptr )
>+int XpuGetPrinter2( const char *printer, char *display, Display **pdpyptr, XPContext *pcontextptr )
> {
>     Display   *pdpy;
>     XPContext  pcontext;
>@@ -90,13 +91,13 @@
>         int           list_count;
>         
>         /* get list of available printers... */
>-        list = XpGetPrinterList(pdpy, printer, &list_count);        
>+        list = XpGetPrinterList(pdpy, (char *)printer, &list_count);        

You just cast a |const char *| to a |char *|.  That implies that you could end up writing to read-only memory.  In fact, in the function declaration above you change the |char *| to a |const char *|.  Are you sure you want to do that?

>         if( list != NULL ) XpFreePrinterList(list);
>         
>         /* ...and check if printer exists... */
>         if( (list != NULL) && (list_count > 0) )
>         {
>-          if( (pcontext = XpCreateContext(pdpy, printer)) != (XPContext)NULL )
>+          if( (pcontext = XpCreateContext(pdpy, (char *)printer)) != (XPContext)NULL )

Same as above.

>           {
>             *pdpyptr     = pdpy;
>             *pcontextptr = pcontext;
>@@ -187,86 +188,20 @@
> void XpuSetOneAttribute( Display *pdpy, XPContext pcontext, 
>                          XPAttributes type, const char *attribute_name, const char *value, XPAttrReplacement replacement_rule )
> {
>-    char *buffer = 
>-#ifdef XPU_USE_NSPR
>-    PR_Malloc((PRUint32)
>-#else    
>-    Xmalloc(
>-#endif
>-    strlen(attribute_name)+strlen(value)+8);
>+    char *buffer = malloc(strlen(attribute_name)+strlen(value)+8);

What is the magic +8 on this malloc?  If it's required for something please put a comment above the allocation.  Magic numbers are bad.

>     
>     if( buffer != NULL )
>     {
>       sprintf(buffer, "%s: %s", attribute_name, value);      
>       XpSetAttributes(pdpy, pcontext, type, buffer, replacement_rule);
>-#ifdef XPU_USE_NSPR
>-      PR_Free
>-#else
>-      XFree
>-#endif      
>-      (buffer);
>+      free(buffer);
>     }  
> }
> 
>-
>-/* enumerate attribute values - work in porgress */
>-char *XpuEmumerateXpAttributeValue( char *value, void **context )
>-{
>-    /* BUG: This does not work for quoted attribute values
>-     * A good example is the enumeration of supported paper sizes: 
>-     * "{'' {na-letter False {6.3500 209.5500 6.3500 273.0500}} {executive False {6.3500 177.7500 6.3500 260.3500}} {na-legal False {6.3500 209.5500 6.3500 349.2500}} {iso-a4 False {6.3500 203.6500 6.3500 290.6500}} {iso-a3 False {6.3500 290.6500 6.3500 413.6500}} {iso-designated-long False {6.3500 103.6500 6.3500 213.6500}} {na-number-10-envelope False {6.3500 98.4500 6.3500 234.9500}} }"       {'' 
>-         {  
>-           na-letter 
>-           False 
>-           {6.3500 209.5500 6.3500 273.0500}
>-         } 
>-         { 
>-           executive 
>-           False 
>-           {6.3500 177.7500 6.3500 260.3500}
>-         } 
>-         {
>-           na-legal 
>-           False 
>-           {6.3500 209.5500 6.3500 349.2500}
>-         } 
>-         {
>-           iso-a4 
>-           False 
>-           {6.3500 203.6500 6.3500 290.6500}
>-         } 
>-         {
>-           iso-a3
>-           False
>-           {6.3500 290.6500 6.3500 413.6500}
>-         }
>-         {
>-           iso-designated-long 
>-           False 
>-           {6.3500 103.6500 6.3500 213.6500}
>-         } 
>-         {
>-           na-number-10-envelope 
>-           False 
>-           {6.3500 98.4500 6.3500 234.9500}
>-         } 
>-       }
>-     
>-     * ToDo: 
>-     * 1. Write a parser which grabs the 1st level of bracket pairs and 
>-     * enumerate these items (six items in this example).
>-     * 2. Write enumeration functions for single attribute types with 
>-     * complex content.
>-     */
>-    return( (char *)strtok_r(value, " ", (char **)context) );
>-}
>-
>-
> /* check if attribute value is supported or not */
> int XpuCheckSupported( Display *pdpy, XPContext pcontext, XPAttributes type, const char *attribute_name, const char *query )
> {
>-    char *value,
>-         *s;
>+    char *value;
>     void *tok_lasts;
>     
>     value = XpGetOneAttribute(pdpy, pcontext, type, (char *)attribute_name);
>@@ -275,16 +210,20 @@
>     
>     if( value != NULL )
>     {
>+      const char *s;
>+      
>       for( s = XpuEmumerateXpAttributeValue(value, &tok_lasts) ; s != NULL ; s = XpuEmumerateXpAttributeValue(NULL, &tok_lasts) )
>       {
>         XPU_DEBUG_ONLY(printf("XpuCheckSupported: probing '%s'=='%s'\n", XPU_NULLXSTR(s), XPU_NULLXSTR(query)));
>         if( !strcmp(s, query) )
>         {
>           XFree(value);
>+          XpuDisposeEmumerateXpAttributeValue(&tok_lasts);
>           return(1);
>         }  
>       }
>       
>+      XpuDisposeEmumerateXpAttributeValue(&tok_lasts);
>       XFree(value);
>     }  
>     
>@@ -442,4 +381,292 @@
>   return False;
> }
> 
>+static
>+const char *search_matching_bracket(const char *start)
>+{
>+  const char *s     = start;
>+  int         level = 0;
>+  
>+  if (!start)
>+    return(NULL);
>+    
>+  for(;;)
>+  {
>+    switch(*s++)
>+    {
>+      case '\0': return(NULL);
>+      case '{': level++; break;
>+      case '}': level--; break;
>+    }
>+    
>+    if (level == 0)
>+    {
>+      return(s);
>+    }
>+  }
>+}
>+
>+
>+static
>+const char *search_next_space(const char *start)
>+{
>+  const char *s     = start;
>+  int         level = 0;
>+  
>+  if (!start)
>+    return(NULL);
>+    
>+  for(;;)
>+  {   
>+    if(isspace(*s))
>+      return(s);
>+
>+    if(*s=='\0')
>+      return(NULL);      
>+
>+    s++;
>+  }
>+}
>+
>+/* PRIVATE context data for XpuEmumerateXpAttributeValue() */
>+typedef struct _XpuAttributeValueEnumeration
>+{
>+  char *value;
>+  char *start;
>+  char *s;
>+} XpuAttributeValueEnumeration;
>+
>+const char *XpuEmumerateXpAttributeValue( const char *value, void **vcptr )
>+{
>+    XpuAttributeValueEnumeration **cptr = (XpuAttributeValueEnumeration **)vcptr;
>+    XpuAttributeValueEnumeration  *context;
>+    
>+    if (!cptr)
>+      return(NULL);
>+      
>+    if (value)
>+    {
>+      XpuAttributeValueEnumeration *e;
>+      const char *s = value;
>+    
>+      e = malloc(sizeof(XpuAttributeValueEnumeration));
>+      if (!e)
>+        return NULL;
>+    
>+      /* skip leading '{'. */
>+      while(*s=='{')
>+        s++;
>+      /* skip leading blanks or '\''. */
>+      while(isspace(*s) || *s=='\'')
>+        s++;
>+         
>+      e->start = e->s = e->value = strdup(s);    

Are you sure that 's' has a length?  It might be a NULL or might be zero-length, right?

>+    
>+      *cptr = e;
>+    }
>+    
>+    context = *cptr;
>+    
>+    if (!context || !context->s)
>+      return(NULL);
>+     
>+    /* Skip leading blanks, '\'' or '}' */
>+    while(isspace(*(context->s)) || *(context->s)=='\'' || *(context->s)=='}' )
>+      context->s++;
>+
>+    if (*(context->s) == '\0')
>+      return(NULL);
>+
>+    context->start = context->s;
>+    if (*(context->start) == '{')
>+      context->s = (char *)search_matching_bracket(context->start);
>+    else
>+      context->s = (char *)search_next_space(context->start);
>+      
>+    /* end of string reached ? */
>+    if (context->s)
>+    {   
>+      *(context->s) = '\0';
>+      context->s++;
>+    }
>+    
>+    return(context->start);   
>+}
>+
>+
>+void XpuDisposeEmumerateXpAttributeValue( void **vc )
>+{ 
>+  if (vc)
>+  {
>+    XpuAttributeValueEnumeration *context = *((XpuAttributeValueEnumeration **)vc);
>+    free(context->value);
>+    free(context);   
>+  }
>+}
>+
>+/* parse a paper size string 
>+ * (example: '{na-letter False {6.3500 209.5500 6.3500 273.0500}}') */
>+Bool XpuParseMediumSourceSize( const char *value, 
>+                               const char **media_name, Bool *mbool, 
>+                               float *ma1, float *ma2, float *ma3, float *ma4 )
>+{
>+    const char *s;
>+    char       *d, 
>+               *name;
>+    char       *boolbuf;
>+    size_t      value_len;
>+    
>+    if (value && value[0]!='{' && value[0]!='\0')
>+      return(False);
>+      
>+    value_len = strlen(value);
>+    
>+    /* alloc buffer for "media_name" and |boolbuf| in one step */  
>+    name = malloc(value_len*2 + 4);
>+    boolbuf = name + value_len+2;

More magic numbers.  Please comment these.

>+    
>+    /* remove '{' && '}' */
>+    s = value;
>+    d = name;
>+    do
>+    {
>+      *d = tolower(*s);
>+      
>+      if (*s!='{' && *s!='}')
>+        d++;
>+      
>+      s++;  
>+    }
>+    while(*s);
>+      
>+    /* seperate media name from string */
>+    d = (char *)search_next_space(name);
>+    if (!d)
>+    {
>+      free(name);
>+      return(False);
>+    }  
>+    *d = '\0';
>+    *media_name = name;
>+    
>+    /* ... continue to parse the remaining string... */
>+    d++;
>+    
>+    if (sscanf(d, "%s %f %f %f %f", boolbuf, ma1, ma2, ma3, ma4) != 5)
>+    {
>+      free(name);
>+      return(False);
>+    }
>+    
>+    if (!strcmp(boolbuf, "true"))
>+      *mbool = True;
>+    else if (!strcmp(boolbuf, "false"))
>+      *mbool = False;
>+    else
>+    {
>+      free(name);
>+      return(False);    
>+    }
>+          
>+    return(True);
>+}

This function doesn't seem to tolerate errors in the description very well.  For example I suspect that if you have something like:

{na-letter False{6.3500 209.5500 6.3500 273.0500}}

it's going to fail.

>+
>+/* future: Migrate this functionality into |XpGetPrinterList| - just do
>+ * not pass a |Display *| to |XpGetPrinterList|
>+ */
>+XPPrinterList XpuGetPrinterList( const char *printer, int *res_list_count )
>+{
>+    XPPrinterRec *rec = NULL;
>+    int           rec_count = 1; /* allocate one more XPPrinterRec structure
>+                                  * as terminator */
>+    char         *sl;
>+    
>+    if(!res_list_count)
>+      return(NULL);
>+    
>+    sl = strdup(XpuGetXpServerList());
>+      
>+    if( sl != NULL )
>+    {
>+      char *display;
>+      char *tok_lasts;
>+      
>+      for( display = (char *)strtok_r(sl, " ", &tok_lasts) ; 
>+           display != NULL ; 
>+           display = (char *)strtok_r(NULL, " ", &tok_lasts) )

Do all of our platforms have strtok_r?  I can't remember.

>+      {
>+        Display *pdpy;
>+        
>+        if(pdpy = XOpenDisplay(display))
>+        {
>+          XPPrinterList list;
>+          int           list_count;
>+          size_t        display_len = strlen(display);
>+
>+          /* get list of available printers... */
>+          list = XpGetPrinterList(pdpy, (char *)printer, &list_count);        

Another scary cast.

>+        
>+          if(list && list_count)
>+          {
>+            int i;
>+            
>+            for( i = 0 ; i < list_count ; i++ )
>+            {
>+              char *s;
>+              rec_count++;             
>+              rec = realloc(rec, sizeof(XPPrinterRec)*rec_count);
>+              if(!rec) /* failure */
>+                break;
>+                
>+              s = malloc(strlen(list[i].name)+display_len+4);
>+              sprintf(s, "%s@%s", list[i].name, display);
>+              rec[rec_count-2].name = s;
>+              rec[rec_count-2].desc = (list[i].desc)?(strdup(list[i].desc)):(NULL);
>+            }
>+            
>+            XpFreePrinterList(list);
>+          }
>+                   
>+          XCloseDisplay(pdpy);
>+        }
>+      }
>+      
>+      free(sl);
>+    }
>+    
>+    if(rec)
>+    {
>+      /* users: DO NOT COUNT ON THIS DETAIL 
>+       * (this is only to make current impl. of XpuFreePrinterList() easier) */
>+      rec[rec_count-1].name = NULL;
>+      rec[rec_count-1].desc = NULL;
>+      rec_count--;
>+    }
>+    else
>+    {
>+      rec_count = 0;
>+    }
>+      
>+    *res_list_count = rec_count;
>+    return(rec);
>+}      
>+
>+
>+void XpuFreePrinterList( XPPrinterList list )
>+{
>+  if (list)
>+  {
>+    XPPrinterList curr = list;
>+    
>+    while(curr->name!=NULL)
>+    {
>+      free(curr->name);
>+      free(curr->desc);
>+      curr++;
>+    }
>+    
>+    free(list);
>+  }
>+}
>+
> /* EOF. */
>diff -r -u original/mozilla/gfx/src/xprint/xprintutil.h mozilla/gfx/src/xprint/xprintutil.h
>--- original/mozilla/gfx/src/xprint/xprintutil.h	Tue Jul 17 20:54:48 2001
>+++ mozilla/gfx/src/xprint/xprintutil.h	Wed Sep 19 14:17:36 2001
>@@ -72,7 +72,6 @@
> int XpuGetPrinter( const char *printername, Display **pdpyptr, XPContext *pcontextptr );
> void XpuSetOneAttribute( Display *pdpy, XPContext pcontext, 
>                          XPAttributes type, const char *attribute_name, const char *value, XPAttrReplacement replacement_rule );
>-char *XpuEmumerateXpAttributeValue( char *value, void **context );
> int XpuCheckSupported( Display *pdpy, XPContext pcontext, XPAttributes type, const char *attribute_name, const char *query );
> int XpuSetJobTitle( Display *pdpy, XPContext pcontext, const char *title );
> int XpuSetContentOrientation( Display *pdpy, XPContext pcontext, XPAttributes type, const char *orientation );
>@@ -82,6 +81,14 @@
> void XpuWaitForPrintNotify( Display *pdpy, int detail );
> Bool XpuSetResolution( Display *pdpy, XPContext pcontext, long dpi );
> Bool XpuGetResolution( Display *pdpy, XPContext pcontext, long *dpi );
>+const char *XpuEmumerateXpAttributeValue( const char *value, void **vcptr );
>+void XpuDisposeEmumerateXpAttributeValue( void **vc );
>+Bool XpuParseMediumSourceSize( const char *value, 
>+                               const char **media_name, Bool *mbool, 
>+                               float *ma1, float *ma2, float *ma3, float *ma4 );
>+XPPrinterList XpuGetPrinterList( const char *printer, int *res_list_count );
>+void XpuFreePrinterList( XPPrinterList list );
>+
> 
> #define XpuGetJobAttributes( pdpy, pcontext )     XpGetAttributes( (pdpy), (pcontext), XPJobAttr )
> #define XpuGetDocAttributes( pdpy, pcontext )     XpGetAttributes( (pdpy), (pcontext), XPDocAttr )

Comment 13

[Roland Mainz]

Attachment: New patch for 2001-09-28-08-trunk per blizzard's comments

Comment 14

[Roland Mainz]

blizzard wrote:        
> >         /* get list of available printers... */
> >-        list = XpGetPrinterList(pdpy, printer, &list_count);        
> >+        list = XpGetPrinterList(pdpy, (char *)printer, &list_count);        
> 
> You just cast a |const char *| to a |char *|.  
> That implies that you could end up writing to read-only memory.  
> In fact, in the function declaration above you change the |char *| 
> to a |const char *|.  
> Are you sure you want to do that?

_YES_.
 The current version of the X11 headers simply does not use |const| where it
should use it, that's all.
I tested it with Rational Purify and checked the X.org sources, too. 
No problems here...

[snip]
> >- if( (pcontext = XpCreateContext(pdpy, printer)) != (XPContext)NULL )
> >+ if( (pcontext = XpCreateContext(pdpy, (char *)printer)) != (XPContext)NULL)
> 
> Same as above.

See comment above... :-)
It's safe.

[snip]
> >-    strlen(attribute_name)+strlen(value)+8);
> >+    char *buffer = malloc(strlen(attribute_name)+strlen(value)+8);
> 
> What is the magic +8 on this malloc?  If it's required for something please 
> put a comment above the allocation.  Magic numbers are bad.

That's no magic number, that's some space required for the sprintf() statement
below.
I've added a comment above the |malloc()|...
Fixed.

[snip]
> >+const char *XpuEmumerateXpAttributeValue( const char *value, void **vcptr )
> >+{
[snip]     
> >+      e->start = e->s = e->value = strdup(s);    
> 
> Are you sure that 's' has a length?  It might be a NULL or might be 
> zero-length, right?

No. All the "input" strings are obtained from the Xprt server. It parses it's
config and stores the info in an internal database and recreates the strings
from the database if the application quiries them via the Xprint extension,
therefore it is (more or less) guranteed that the strings are correctly
formattet.

[snip]  
> >+    /* alloc buffer for "media_name" and |boolbuf| in one step */  
> >+    name = malloc(value_len*2 + 4);
> >+    boolbuf = name + value_len+2;
> 
> More magic numbers.  Please comment these.

Fixed.

[snip]        
> >+    return(True);
> >+}
> 
> This function doesn't seem to tolerate errors in the description very well.  
> For example I suspect that if you have something like:
> {na-letter False{6.3500 209.5500 6.3500 273.0500}}
> 
> it's going to fail.

1. See comment about the strings returned by Xprt (via X print extension). Both
the X.org implementation and the JAVA Xprt version return only correctly
formattet strings here.
2. Xp tokes _must_ be seperated by whitespaces except for the first token after
'{' and the token before '}'
3. It may be usefull to replace the whole parser stuff with a LEX/YACC version,
but I think it adds more trouble than it's worth (question is how portable the
LEX/YACC stuff is...).

[snip]
> >+      for( display = (char *)strtok_r(sl, " ", &tok_lasts) ; 
> >+           display != NULL ; 
> >+           display = (char *)strtok_r(NULL, " ", &tok_lasts) )
> 
> Do all of our platforms have strtok_r?  I can't remember.

The code already uses |strtok_r| in other places, no problem here.

[snip]
> >+          size_t        display_len = strlen(display);
> >+
> >+          /* get list of available printers... */
> >+          list = XpGetPrinterList(pdpy, (char *)printer,
&list_count);        
> 
> Another scary cast.

See above...

Wanna give me a rs= for the new patch, please ?

Comment 15

[Christopher Blizzard (:blizzard)]

>> > /* get list of available printers... */
>> >-        list = XpGetPrinterList(pdpy, printer, &list_count);        
>> >+        list = XpGetPrinterList(pdpy, (char *)printer, &list_count);        
>> > You just cast a |const char *| to a |char *|.  
>> That implies that you could end up writing to read-only memory.  
>> In fact, in the function declaration above you change the |char *| 
>> to a |const char *|.  
>> Are you sure you want to do that?
> 
> _YES_.
>  The current version of the X11 headers simply does not use |const| where it
> should use it, that's all.
> I tested it with Rational Purify and checked the X.org sources, too. 
> No problems here...
> 
> 

You should be relying on the API, not the fact that the implementation happens
to use it like it was const.  That implementation might change down the road. 
Please fix this.

Comment 16

[Roland Mainz]

Attachment: New patch for 2001-10-02-08-trunk

Comment 17

[Asa Dotzler [:asa]]

0.9.5 is out the door. bumping TM up by one.

Comment 18

[Brendan Eich [:brendan]]

WRITABLE is spelled without an E.

Don't cast away const, do use a char * temporary to point to the strdup return
value, do check for null (out of memory).

Why do we need +250 lines of code for this infrastructure?  Just curious.

/be

Comment 19

[Mike Shaver (:shaver emeritus)]

I can't find any examples (outside of xprintutil.c) that calls strtok_r without
protecting it with #if HAVE_STRTOK_R.  (And read bug 99248 before you just go
plaster it with that #if and fall back to strtok(3).)

(strtok_r is part of POSIX.4a, and I'm not sure how widely that's supported.  I
fear the VMS compiler here.)

Comment 20

[Roland Mainz]

Attachment: New patch for 2001-10-15-08-trunk per brendan's and shaver's comments

Comment 21

[Roland Mainz]

Brendan Eich wrote:
> WRITABLE is spelled without an E.
>
> Don't cast away const, do use a char * temporary to point to the strdup return
> value, 

Uhm... this is C code, not C++...

The |MAKE_STRING_WRITABLE| macro&co. is only "dummy-conformance" stuff. The X11
headers to not make use of |const| where they should use it - and to conform the 
headers I added the macros (see comment in source).
I fact we can |#define MAKE_STRING_WRITABLE(str)| and it would hurt noone (even
not Rational Purify)... =:-)

Maybe someone ask OpenGroup to fix the xx@@!!!-headers...

> Why do we need +250 lines of code for this infrastructure?  Just curious.

That's intrastructure for patches in bug 84947 (the new Unix+OS2 print dialog).
I filed a seperate bug for this to keep the patches small...

----

shaver wrote:
> I can't find any examples (outside of xprintutil.c) that calls strtok_r 
> without protecting it with #if HAVE_STRTOK_R. 

Mhhh... OK...
I added an own version of strtok_r for the case that |HAVE_STRTOK_R| is not set.

> (And read bug 99248 before 
> you just go plaster it with that #if and fall back to strtok(3).)
> (strtok_r is part of POSIX.4a, and I'm not sure how widely that's supported.  
> I fear the VMS compiler here.)

strtok!=strtok_r. I am using strtok_r in a nested way which makes it impossible
to replace it with strtok. I added a fallback (see above).

Comment 22

[Roland Mainz]

Attachment: New patch for 2001-10-15-08-trunk

Comment 23

[Brendan Eich [:brendan]]

This is not a full review, because I'd like to see a revised patch that fixes
these issues, however minor they seem:

1.  The first line of xprintutil.c is empty, please replace it with the correct
Emacs modeline comment.

2.  That modeline comment should specify the indentation unit in spaces, but you
seem to mix four- and two-space indentation -- please pick one and stick to it.
 For example, search_matching_bracket starts with 4, then switches to 2 after
one level.

3.  my_strtok_r should test 'if (c == 0)' right after loading 'c = *s1++;',
since c can never match sc if c is 0, because sc cannot be 0.  Also, put the
spanp++ in the update (third) part of the for loop header, where it belongs!

4.  MAKE_WRITABLE_STRING overparenthesizes its definition once, and sets str to
null in the "else" (after :) part of the ternary operator for no goood reason.
Use ((str) == NULL || ((str) = strdup(str))) instead of ?:.

5.  WRITABLE is still misspelled in FREE_WRITEABLE_STRING.

6.  STRING_IS_WRITABLE seems misnamed, how about STRING_AS_WRITABLE or even
WRITABLE_STRING?  But again, I believe the code would be cleaner if you used a
char* variable and avoided casting away const.  Your call.

7.  Why do you write an infinite loop in search_matching_bracket?  It should be
a do {...} while (level != 0) loop.  Also, maybe skip_matching_brackets is a
better name, given the return value?

All for now, blizzard should have a look.

/be

Comment 24

[Roland Mainz]

> 1.  The first line of xprintutil.c is empty, please replace it with the 
> correct Emacs modeline comment.

Fixed in all xprintutil* files ...

> 2.  That modeline comment should specify the indentation unit in spaces, but 
> you seem to mix four- and two-space indentation --
> please pick one and stick to it.

Well... first indentation within function/method = 4, further indentation = 2.

> For example, search_matching_bracket starts with 4, then switches to 2
> after one level.

Correct. Company coding style... I am using it for years now... 
However - I fixed it to 2/2 to make you happy... :-)

> 3.  my_strtok_r should test 'if (c == 0)' right after loading 'c = *s1++;',
> since c can never match sc if c is 0, because sc cannot be 0.  Also, put the
> spanp++ in the update (third) part of the for loop header, where it belongs!

I moved the strtok_r() issue to a seperate bug - currenty Mozilla uses three
versions of the string tokenizer instead of one. I filed bug 106372 (and
attached a patch) to get a NSPR version of strtok_r() and then (later) move all
code over to use the NSPR version...

> 4.  MAKE_WRITABLE_STRING overparenthesizes its definition once, and sets str 
> to null in the "else" (after :) part of the ternary operator for no goood 
> reason.
> Use ((str) == NULL || ((str) = strdup(str))) instead of ?:.

I changed it to (((str)?((str) = strdup(str)):0) - the contruct above is nice
but may force people to thing twice before they understand it's meaning... :-)

> 5.  WRITABLE is still misspelled in FREE_WRITEABLE_STRING.

Fixed.

> 6.  STRING_IS_WRITABLE seems misnamed, how about STRING_AS_WRITABLE or even
> WRITABLE_STRING?  But again, I believe the code would be cleaner if you used a
> char* variable and avoided casting away const.  Your call.

I changed it to STRING_AS_WRITABLE ...

> 7.  Why do you write an infinite loop in search_matching_bracket?  It should 
> be a do {...} while (level != 0) loop.

Fixed.

> Also, maybe skip_matching_brackets is a
> better name, given the return value?

Yup. Fixed.

Comment 25

[Roland Mainz]

Attachment: gdiff -w -u version of attachment 55437 (review only)

Comment 26

[Roland Mainz]

Attachment: New patch for 2001-10-26-08-trunk

Comment 27

[Christopher Blizzard (:blizzard)]

Comment on attachment 55441 [details] [diff] [review]
gdiff -w -u version of attachment 55437 [details] [diff] [review] (review only)

sr=blizzard

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 55441 [details] [diff] [review]
gdiff -w -u version of attachment 55437 [details] [diff] [review] (review only)

r=[e-mail] with the proviso
that you remove any tabs (I can't be sure 
but there might be some; it's hard to tell
from diffs.)

Comment 29

[Roland Mainz]

There are no TABs in the patch - but "gdiff -w" "ignores" whitespace changes
which makes the patch look ugly.
http://bugzilla.mozilla.org/attachment.cgi?id=55437&action=view is the same
patch (for checkin) where the whitespace stuff is OK ...

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Fix checked in for gisburn

Comment 31

[Masaki Katakai]

Marked this as VERIFIED. New print dialog works fine for me even
for Xprint.