[Nut-upsdev] upsdrv_print_ups_list and afferant reflections

[Peter Selinger]

Hi Jonathan,

I had a brief look at the parser. Here are some cases where I think
the parser will malfunction:

Name: APC Back-UPS 800         # the parser will set value="APC"
VendorID:0x0001                # the parser will not see the ":"
ProductID: 0002		       # missing "0x"
====                           # the parser will not recognize this line
Name: XYZ UPS 1000
VendorID: 0x0003
ProductID: 0x0004
===
Name: XYZ UPS 1001
VendorID: 0x0005               # since productID != 0 (line 152), the
                               # parser will call print_ups(0005,0004), a bug
ProductID: 0x0006              # since vendorID != 0 (line 165), the
                               # parser will call print_ups(0005,0006)

Is it really appropriate to use the PCONF_CTX macros for parsing a
key-value file format? Perhaps it would be better to have a function
strip() (destructively remove white space, i.e., ' \t\n', from front
and end of a string), and a function key_value, perhaps as follows:

/* if line represents a key-value pair, return 1 and let key and value
   point to key and value. "line" is updated destructively, and key
   and value may point inside line. If not a key-value-pair, return 0
   and leave line unchanged. */
int key_value(char *line, char **key, char **value) {
    char *p;

    p = strchr(line, ':');
    if (!p) {
       return 0;
    }
    *p = 0;
    *key = strip(line);
    *value = strip(value);
    return 1;
}

Then the parse() function could be something simple and reliable, like:

void parse(FILE *pipe) {
     char line[100];
     char *p, *key, *value;
     int l = 0;
     int productid = 0, vendorid = 0, ...;
     int have_record = 0;

     while (getline(line, 100, pipe) >= 0) {
	 l++;
	 p = strip(line);
	 if (*p == 0 || *p == '#') { /* comment, blank line */
	    continue;
         } else if (key_value(p, &key, &value)) {
	    if (strcasecmp(key, "ProductID") == 0) {
	       productid = parse_hex(value);
            } else if (strcasecmp(key, "VendorID") == 0) {
	       vendorid = parse_hex(value);
            } else if (...) {
	       ...
            } else {
	       upsdebugx("Warning: unknown key %s in line %d\n", key, l);
            }
	    have_record = 1;
	    continue;
         } else if (*p == '=') {  /* any line starting with '=' */
	    if (have_record) {
	        print_record(vendorid, productid, ...);
            }
         }
     }
     if (have_record) {
	 print_record(vendorid, productid, ...);
     }
}

This would treat multi-word values, and all the other special cases,
correctly, and would also extend easily to new key-value pairs. 

-- Peter

Jonathan Dion wrote:
> 
> > > MyDriver version x.y.z...
> > >
> > > List of Supported UPSs
> > > ===
> > > Name : UPS1
> > > Vendor : TheVendor
> > > Extra : USB
> > > VendorID : 0xXXXX
> > > ProductID : 0xXXXX
> > > Status : Supported
> > > ===
> > > Name UPS2
> > > Vendor : TheVendor
> > > Status : Unstable
> > > ===
> > > .
> > > .
> > > .
> >
> > Make sure you make the format flexible enough that: (1) all whitespace
> > before/after ":" and at the beginning/end of the line is ignored
> > (there should be no difference between "Name: X" and " Name : X "),
> > (2) case is ignored in keywords (vendorID, Vendorid etc), (3) blank
> > lines are ignored, (4) include some format for comments, and (5) if
> > lines of the form "===" have a special meaning, make the rule for this
> > flexible (e.g., any line starting with "=" after whitespace).
> >
> > In short, even though this format is machine-generated, it should still
> > be human-editable, just in case.
> 
> No problem for all that. It is done.
> # is the comments escape character