Maintainance cleanup #20

jnpkrn · 2016-02-17T16:01:57Z

No description provided.

jnpkrn · 2016-02-22T14:40:15Z

Related question, was there significant reason to use unsigned char
specifically? The issue is that string functions use unqualified char
(whether it is signed or not is implementation specific, moreover
typeof((char) 'c') != typeof((unsigned char) 'c') on major platforms)
hence the combination with variables derived from unsigned char makes
the compilers complain a lot.

Any objections against dropping unsigned qualifiers in this context?

dmuhamedagic · 2016-02-22T14:43:31Z

On Mon, Feb 22, 2016 at 06:40:15AM -0800, Jan Pokorný wrote:

Related question, was there significant reason to use unsigned char
specifically?

Where?

jnpkrn · 2016-02-22T14:48:56Z

For instance:

src/config.c:912:int find_site_by_name(unsigned char *site, struct booth_site **node, int any_type)
src/booth.h:80:typedef unsigned char boothc_site  [BOOTH_NAME_LEN];
src/booth.h:81:typedef unsigned char boothc_ticket[BOOTH_NAME_LEN];
src/booth.h:82:typedef unsigned char boothc_attr[BOOTH_NAME_LEN];
src/booth.h:83:typedef unsigned char boothc_attr_value[BOOTH_ATTRVAL_LEN];

Then, e.g.,

config.c: In function ‘add_ticket’:
config.c:229:9: warning: pointer targets in passing argument 1 of ‘strcpy’ differ in signedness [-Wpointer-sign]
  strcpy(tk->name, name);
         ^

dmuhamedagic · 2016-02-22T14:52:16Z

On Mon, Feb 22, 2016 at 06:48:56AM -0800, Jan Pokorný wrote:

For instance:

src/config.c:912:int find_site_by_name(unsigned char *site, struct booth_site **node, int any_type)
src/booth.h:80:typedef unsigned char boothc_site  [BOOTH_NAME_LEN];
src/booth.h:81:typedef unsigned char boothc_ticket[BOOTH_NAME_LEN];
src/booth.h:82:typedef unsigned char boothc_attr[BOOTH_NAME_LEN];
src/booth.h:83:typedef unsigned char boothc_attr_value[BOOTH_ATTRVAL_LEN];

Then, e.g.,

config.c: In function ‘add_ticket’:
config.c:229:9: warning: pointer targets in passing argument 1 of ‘strcpy’ differ in signedness [-Wpointer-sign]
  strcpy(tk->name, name);
         ^

OK. I assume that you mean using 'unsigned char' in general,
right? I suppose that we can use 'char' instead, at least for
string types of the kind you quoted above. Care to take care of
it too?

jnpkrn · 2016-02-22T15:01:38Z

On 22/02/16 06:52 -0800, Dejan Muhamedagic wrote:

On Mon, Feb 22, 2016 at 06:48:56AM -0800, Jan Pokorný wrote:

For instance:

src/config.c:912:int find_site_by_name(unsigned char *site, struct booth_site **node, int any_type)
src/booth.h:80:typedef unsigned char boothc_site  [BOOTH_NAME_LEN];
src/booth.h:81:typedef unsigned char boothc_ticket[BOOTH_NAME_LEN];
src/booth.h:82:typedef unsigned char boothc_attr[BOOTH_NAME_LEN];
src/booth.h:83:typedef unsigned char boothc_attr_value[BOOTH_ATTRVAL_LEN];

Then, e.g.,

config.c: In function ‘add_ticket’:
config.c:229:9: warning: pointer targets in passing argument 1 of ‘strcpy’ differ in signedness [-Wpointer-sign]
  strcpy(tk->name, name);
         ^

OK. I assume that you mean using 'unsigned char' in general,
right?

Yep (didn't want the description to get too hairy).

I suppose that we can use 'char' instead, at least for string types
of the kind you quoted above. Care to take care of it too?

Will do. The only cases that would matter is when such char is promoted
to int and used in the arithmetics, so I will double check.

Jan (Poki)

dmuhamedagic · 2016-02-22T15:10:58Z

On Mon, Feb 22, 2016 at 07:01:38AM -0800, Jan Pokorný wrote:

On 22/02/16 06:52 -0800, Dejan Muhamedagic wrote:
On Mon, Feb 22, 2016 at 06:48:56AM -0800, Jan Pokorný wrote:
For instance:
src/config.c:912:int find_site_by_name(unsigned char *site, struct booth_site **node, int any_type)
src/booth.h:80:typedef unsigned char boothc_site  [BOOTH_NAME_LEN];
src/booth.h:81:typedef unsigned char boothc_ticket[BOOTH_NAME_LEN];
src/booth.h:82:typedef unsigned char boothc_attr[BOOTH_NAME_LEN];
src/booth.h:83:typedef unsigned char boothc_attr_value[BOOTH_ATTRVAL_LEN];
Then, e.g.,
config.c: In function ‘add_ticket’:
config.c:229:9: warning: pointer targets in passing argument 1 of ‘strcpy’ differ in signedness [-Wpointer-sign]
  strcpy(tk->name, name);
         ^
OK. I assume that you mean using 'unsigned char' in general,
right?
Yep (didn't want the description to get too hairy).

I suppose that we can use 'char' instead, at least for string types
of the kind you quoted above. Care to take care of it too?

Will do. The only cases that would matter is when such char is promoted
to int and used in the arithmetics, so I will double check.

I couldn't find any such case, but please check. Thanks!

jnpkrn · 2016-04-05T14:11:23Z

@dmuhamedagic Can I start rehashing this PR with respect to your objections?

dmuhamedagic · 2016-04-05T14:12:41Z

Yes, please.

References: http://rpm.org/gitweb?p=rpm.git;a=commit;h=3fc5824 (~2009)

Note that, furthermore, cl_log is not in "libgpl" (namesake does exist, though: Game Programming Library), but rather in "libplumb" that is currently referenced in src/Makefile.am directly, anyway.

...as this is the only reference configure script uses. References: http://rpm.org/gitweb?p=rpm.git;a=commit;h=6c4b0fc (~2007)

Respective code removed as of f5700b4

Also make use of typedef'd boothc_site.

Some cases might have still been omitted.

Or more frankly, where compiler complains (which is a good approximation) + human spotted eligible parts. Also make cast from/to "xmlChar *" explicit.

jnpkrn · 2016-04-11T21:32:27Z

So currently I offer a new round for the pull with the following changes:

9d8f023 commit using pkgconfig(glib-2.0) replaced with
ceec5c2 that does that everywhere except for SuSE based distros
to avoid unresolvable dependency with SLE11SP4
5c91c50 and da6b1d6 commits excluded as requested and as documented
at https://en.opensuse.org/openSUSE:Specfile_guidelines#Metadata_Tags,
respectively

Or more frankly, where compiler complains, which is a good approximation.

dmuhamedagic · 2016-04-18T14:57:30Z

src/attr.c

 	if (!notime)
 		get_time(&a->update_ts);

+	assert(strnlen(name, BOOTH_NAME_LEN) < BOOTH_NAME_LEN);


I'd rather have booth continue to run, even with a truncated attribute name. iirc, this was not unpremeditated.

OK, I understand that it might be too easy (accidentally or not) to push booth layer out of the function with a single crafted geo attribute in CIB/crm_ticket one-liner. So does turning that into warning sounds better? Then, we should likely warn also in case of trying to set/delete such a troublesome (too long in either name or value) attribute being provided by the user to geostore command.

On Tue, Apr 19, 2016 at 06:26:49AM -0700, Jan Pokorný wrote:

@@ -274,8 +277,9 @@ int store_geo_attr(struct ticket_config *tk, const char *name, char *val, int no
if (!notime)
get_time(&a->update_ts);

assert(strnlen(name, BOOTH_NAME_LEN) < BOOTH_NAME_LEN);

OK, I understand that it might be too easy (accidentally or not) to push booth layer out of the function with a single crafted geo attribute in CIB/crm_ticket one-liner. So does turning that into warning sounds better? Then, we should likely warn also in case of trying to set/delete such a troublesome (too long in either name or value) attribute being provided by the user to geostore command.

Yes, warning seems to be better.

jnpkrn · 2016-04-22T09:14:17Z

Let's close this monolithic PR in favor of split PRs: #28, #29, #30
(should preferably be pulled in-order).
These PRs reflect the discussion so far.

jnpkrn force-pushed the maint-cleanup branch 2 times, most recently from 25311e6 to 7f50b96 Compare February 17, 2016 18:05

jnpkrn force-pushed the maint-cleanup branch from 7491fd8 to 6936d18 Compare February 24, 2016 14:38

jnpkrn mentioned this pull request Feb 24, 2016

Feature: offer glue alternatives #22

Merged

jnpkrn force-pushed the maint-cleanup branch from 6936d18 to 1b7a442 Compare February 24, 2016 22:44

jnpkrn added 15 commits April 11, 2016 23:02

build: s/CPPFLAGS/CFLAGS/ + use GLIB_LIB

4e4a3ec

maint: spec: SPDX license notation only for SUSE

056b9c9

maint: spec: drop redundant, unused %check scriptlet

e40d4a3

maint: spec: get rid of redundant %clean section

ebf53e3

References: http://rpm.org/gitweb?p=rpm.git;a=commit;h=3fc5824 (~2009)

maint: configure: drop useless AC_CHECK_LIB invocations

ab5f397

Note that, furthermore, cl_log is not in "libgpl" (namesake does exist, though: Game Programming Library), but rather in "libplumb" that is currently referenced in src/Makefile.am directly, anyway.

maint: spec: use pkgconfig for glib-2.0

ceec5c2

...as this is the only reference configure script uses. References: http://rpm.org/gitweb?p=rpm.git;a=commit;h=6c4b0fc (~2007)

maint: configure: nicely formatted help/summary screens

e030217

maint: configure: --enable-resource-monitor no longer relevant

2c22f54

Respective code removed as of f5700b4

maint: build: allow requesting no HTML man + use in spec

1319660

Low: avoid using cl_log directly

5d2b6b3

Low: "char *" more string funcs friendly than unsigned variant

1dcd750

Also make use of typedef'd boothc_site.

Low: attr: assert the string truncation will not occur

b8b9657

Low: make (a defensive) use of known static buffer sizes

ce1effc

Some cases might have still been omitted.

Low: apparently static function should be marked so

89d9e06

Low: const-cast char arrays where suitable

8149495

Or more frankly, where compiler complains (which is a good approximation) + human spotted eligible parts. Also make cast from/to "xmlChar *" explicit.

jnpkrn force-pushed the maint-cleanup branch from 42647a4 to 0e96bf7 Compare April 11, 2016 21:24

Low: use unsigned int where suitable

9c7ccd2

Or more frankly, where compiler complains, which is a good approximation.

jnpkrn force-pushed the maint-cleanup branch from 0e96bf7 to 9c7ccd2 Compare April 12, 2016 15:25

dmuhamedagic reviewed Apr 18, 2016
View reviewed changes

jnpkrn closed this Apr 22, 2016

Maintainance cleanup #20

Maintainance cleanup #20

Uh oh!

Conversation

jnpkrn commented Feb 17, 2016

Uh oh!

jnpkrn commented Feb 22, 2016

Uh oh!

dmuhamedagic commented Feb 22, 2016

Uh oh!

jnpkrn commented Feb 22, 2016

Uh oh!

dmuhamedagic commented Feb 22, 2016

Uh oh!

jnpkrn commented Feb 22, 2016

Uh oh!

dmuhamedagic commented Feb 22, 2016

Uh oh!

jnpkrn commented Apr 5, 2016

Uh oh!

dmuhamedagic commented Apr 5, 2016 via email

Uh oh!

jnpkrn commented Apr 11, 2016

Uh oh!

dmuhamedagic Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

jnpkrn Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

dmuhamedagic Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

jnpkrn commented Apr 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants