Closed metabiswadeep closed 1 year ago
Otherwise it looks all OK now for me. Did you also check that everything uses either public APIs or any public or private API of libppd? No use of private APIs oCUPS and also PPD-related functions onl;y of libppd, not of CUPS. Also no (f)printf()
or similar in library functions (logging via log function there).
Otherwise it looks all OK now for me. Did you also check that everything uses either public APIs or any public or private API of libppd? No use of private APIs oCUPS and also PPD-related functions onl;y of libppd, not of CUPS. Also no
(f)printf()
or similar in library functions (logging via log function there).
According to what I have checked, there is not any function of "-private.h" of cups used in it. Also logging fuction has been used for everything and fprintf() or fputs() has not been used in ppd-test.c. However, puts has been used in testppdfile.h to get the user readable output.
Could you also change all the comments from /* ... */
to // ...
as I have done it this way in the rest of libppd (actually following the coding style of PAPPL and not of CUPS).
Thanks for the changes, but there are some issues:
// ...
for comments always have a space between //
and the comment._ppdArrayAddStrings()
to cupsArrayAdd()
was only needed in testppdfile.c
NOT in ppdTest()
. The latter is inside libppd and therefore can use libppd's private interfaces. Could you change this back?WARN_ALL
-> PPD_TEST_WARN_ALL
...Please also compile the code and test some PPDs with the new utility to check whether you get the same results as with the original utility from CUPS.
Thanks for the changes, but there are some issues:
- When using
// ...
for comments always have a space between//
and the comment.- The change from
_ppdArrayAddStrings()
tocupsArrayAdd()
was only needed intestppdfile.c
NOT inppdTest()
. The latter is inside libppd and therefore can use libppd's private interfaces. Could you change this back?- The values of the enum also need to get renamed:
WARN_ALL
->PPD_TEST_WARN_ALL
...Please also compile the code and test some PPDs with the new utility to check whether you get the same results as with the original utility from CUPS.
Is it not possible to use cupsArrayAdd in ppdTest as I believe it will serve the purpose better as compared to _ppdArrayAddStrings?
What we want is a CUPS array where each element contains one line of the output text. _ppdArrayAddStrings()
allows supplying multi-line strings and they get separated into the individual lines. Duplicate lines are dropped. cupsArrayAdd()
just adds the strings, if each array entry should be a line of the output, you need to take care that each string you add is actually a single line (no \n
). Also, duplicate lines will simply get added.
Did you add instructions for the building of testppdfile
in Makefile.am
? Have a look in Makefile.am
how these instructions look like for other executables contained in the libppd package.
Now, when I run testppdfile, it shows segmentation fault (core dumped). Could you please help me with this error?
First, your addition to the Makefile.am
uses only the LDFLAGS
and CFLAGS
for libppd and libcups, not for libcupsfilters, but I do not really know whether this can be the reason for the segfault, probably rather not.
Second, to investigate a segfault, you have to run the program under gdb
:
LD_LIBRARY_PATH=.libs gdb .libs/testppdfile
At the prompt of gdb
you enter
run file.ppd
where file.ppd
is the PPD which you tested and got the segfault.
Once the segfault happens, you are back at the gdb
prompt and you enter
bt
to get a backtrace. You see now which functions were called and in which line of the caller each function got called. Also some variable values are shown. Now look for the last function called which belongs to libppd or especially your added file(s) and in which line it calls some other function which finally leads to the segfault. The problem is usually in the last line coming from you which the program executed before segfaulting.
Did you create the CUPS array for the human-readable output lines via cupsArrayNew()
? If you add items to a CUPS array which you did not create yet, you will most probably get a segfault.
Thank you very much for your last changes.
Did you find out what caused the segfault and did you succeed to fix it? If yes, how did you fix it.
You also should not do functional changes (fix of segfault) and cosmetical changes (correction of spaces) in the same commit. If you have actually fixed the segfault with this commit, it is impossible for me to find your fix.
The segfault was due to using ppdAddStrings to add the status variable, which was of int datatype to the output array. Removing it solved the error.
Now the output content is the same as cupstestppd, just the order of output statements in the terminal is messed up.
The messed up output is easily explained. You cannot use _ppdArrayNewStrings()
and _ppdArrayAddStrings()
, as the underlying CUPS array is a sorted array. See the source code of the _ppdArrayNewStrings()
in ppd/array.c
. The array is created via cupsArrayNew3()
with strcmp()
as comparison function specified, and if a CUPS array is created with a comparison function, it is sorted, if the comparison function is set as NULL, the array is unsorted, meaning that the items are manually positioned, (cupsArrayAdd()
adds the new item to the end then).
So I recommend you to simply use the standard CUPS array API to build the output list. Create the array with NULL as comparison function and add each line with cupsArrayAdd()
. This also allows to add the empty string if you want to output a blank line as separator.
For cupsArrayAdd(), the issue of ordering is somewhat still prevailing but another problem arises where formatted strings are not displayed but in their place ���������������� is displayed.
Another problem of the _ppdArrayNewStrings()
/_ppdArrayAddStrings()
set of functions is that duplicate strings are not accepted. You will either have to use cupsArrayNew()
/cupsArrayAdd()
or take a pointer to char
and for each new output line you realloc()
this pointer to fit the extra bytes for the new line and copy the new line to the end of the string, so having one string which is growing for each line. You can easily printf()
and free()
this in the end.
For the strings not correctly output from cupsArrayNew()
/cupsArrayAdd()
have you allocated memory for each line and added the pointer to this memory to the array? cupsArrayAdd()
does nothing more than adding a pointer to the array. It does neither allocate memory nor copying the string.
For formatted strings you have to crate the string with snprintf()
in a buffer and then copy it into the memory allocated for the output line.
Using cupsArrayAdd() I have added the string str_format to the output array but since cupsArrayAdd uses adds only a pointer to the array, every time the value of str_format changes, its previous value inserted into the output array also changes, so all the output lines formatted with snprintf is coming to be the same. Could you please tell me how I can avoid this and make sure that the value of the already added strings in the output array does not change?
@tillkamppeter testppdfile is now working same as cupstestppd.
A suggestion to improve the architecture of the ppdTest()
function:
report
of type cupsArray *
. If NULL is supplied no report array is created, if a pointer which is set to NULL is supplied, a CUPS array is created with cupsArrayNew3()
and the pointer is set to pinting to that array, if a non-Null pointer is supplied, it is assumed to be an array of the right type and new lines aget added to it (allows to put several reports, one after the other into one array).if (report && *report == NULL)
{
*report = cupaArrayNew3(...);
if (*report == NULL)
{
...
return (-1);
]
]
ppdTest:
" in the string constant in the first line):
sprintf(str_format, "...", XXX, YYY);
if (*report)
cupsArrayAdd(*report, (void *)str_format);
if (log) log(ld, CF_LOGLEVEL_..., "ppdTest: %s", str_format);
I have edited the code I have suggested in the previous comment, concerning the "ppdTest:
" prefix.
Also with this it is easy for a developer to use the ppdTest()
function as they prefer: Either to simply check the validity of the PPD without any reporting, reporting in the caller's log (by supplying a log function), reporting into the array, for stdout output or for parsing the report for futher analysis ...
A suggestion to improve the architecture of the
ppdTest()
function:
- Make the creation of a human-readable report array optional. For that add to the parameter list (as last item) a parameter named
report
of typecupsArray *
. If NULL is supplied no report array is created, if a pointer which is set to NULL is supplied, a CUPS array is created withcupsArrayNew3()
and the pointer is set to pinting to that array, if a non-Null pointer is supplied, it is assumed to be an array of the right type and new lines aget added to it (allows to put several reports, one after the other into one array).
Instead of making a new report variable can't the output variable be modified to perform this functionality?
I want to make the return value of ppdTest()
an integer, to easily get the result, PASS, FAIL, or error. Therefore I want to have the pointer to the report in the parameter list of ppdTest()
. This also gives flexibility. You can have an existing array, for example containing a previous report, or simply an empty array which you created. Then you simply supply a pointer to this array and ppdTest()
adds its report to the end of this array. You can have a variable which should hold the report. You set it to NULL and supply a pointer to this variable. The ppdTest()
creates the array for you. Or you supply NULL if you do not want to have a report, if the integer result and perhaps also the logging via log function is good enough for your. So this should server everyone ...
I want to make the return value of
ppdTest()
an integer, to easily get the result, PASS, FAIL, or error. Therefore I want to have the pointer to the report in the parameter list ofppdTest()
. This also gives flexibility. You can have an existing array, for example containing a previous report, or simply an empty array which you created. Then you simply supply a pointer to this array andppdTest()
adds its report to the end of this array. You can have a variable which should hold the report. You set it to NULL and supply a pointer to this variable. TheppdTest()
creates the array for you. Or you supply NULL if you do not want to have a report, if the integer result and perhaps also the logging via log function is good enough for your. So this should server everyone ...
If PPD-test does not return the output array( for this matter the report array), then how can it be accessed by the testppdfile to provide the human readable output if the user wants the report to be shown?
I hope you know the difference between call-by-value and call-by-reference. For getting the report back I put cups_array_t **report
and not cups_array_t *report
into the parameter list of ppdTest()
. So I pass the address of a cups_array_t
. So ppdTest()
uses, modifies, or creates the report array on this address. Afterwards the caller reads the array from this address.
You call ppdTest()
as follows then:
cups_array_t *report = NULL;
int result;
...
result = ppdTest(..., &report);
...
if (report)
{
char *line;
for (line = (char *)cupsArrayFirst(output);
line;
line = (char *)cupsArrayNext(output))
puts(line);
}
...
Looks great! Thank you. Did you compile and test it?
Will have another look and if all is fine I will merge it.
Looks great! Thank you. Did you compile and test it?
Yes, I did compile and test it.
OK. Could you do the following corrections:
testppdfile
from the repository. It is a temporary, machine-generated script from libtool. The GIT repo should only contain human-generated files.sprintf()
the output string from a format string and variables, but simply have a constant output string, simply do:
str_format = " **FAIL** REQUIRED DefaultPaperDimension\n"
" REF: Page 103, section 5.15.");
if (*report)
cupsArrayAdd(*report, (void *)str_format);
if (log) log(ld, CF_LOGLEVEL_ERROR, "ppdTest: %s", str_format);
sprintf()
but use snprintf()
. Set as the length the length of the buffer - 1, but do not put the number, put sizeof(str_format) - 1
, so that you can simply redefine the size of str_format
whenever you want, without needing to correct hundreds of lines.CF_LOGLEVEL_ERROR
but also CF_LOGLEVEL_DEBUG
. Use CF_LOGLEVEL_ERROR
only if there is an error with reading the PPD file or another error of ppdTest()
. A failing test result is not considered an error here.ppdTest()
in ppd/ppd.h
.Now it looks all correct, but could you have another look through the indentation? It is 2 spaces per level.
The tab width is 8 spaces (tab character fills up to the next 8-char grid line).
See DEVELOPING.md
.
Your code produces tons of warnings. Could you fix them ASAP (with a new PR)?
Your code produces tons of warnings. Could you fix them ASAP (with a new PR)?
Sure I'll do it.
Your code produces tons of warnings. Could you fix them ASAP (with a new PR)?
I see that the warnings have been taken care of by you. Sorry that I couldn't look into it. I had an exam today so didn't get the time to do it.
Migrated cupstestppd.c from cups to ppd-test.c in libppd to make it legacy.