Skip to contentSkip to conversation

libsmartcols patch

PATCH Phillip Susi 3 participants 6 messages Dec 29, 2014 – Feb 16, 2015 Util-Linux package

lore
Share & tools
lore
#0 lore raw

Forwarding patch from https://launchpad.net/bugs/1406133:

src/table_print.c +53 -28
@@ -151,6 +151,32 @@
 	return bytes;
 }
 
+/* returns pointer to the end of used data */
+static int line_ascii_art_to_buffer(struct libscols_table *tb,
+				    struct libscols_line *ln,
+				    struct libscols_buffer *buf)
+{
+	const char *art;
+	int rc;
+
+	assert(ln);
+	assert(buf);
+
+	if (!ln->parent)
+		return 0;
+
+	rc = line_ascii_art_to_buffer(tb, ln->parent, buf);
+	if (rc)
+		return rc;
+
+	if (list_entry_is_last(&ln->ln_children, &ln->parent->ln_branch))
+		art = "  ";
+	else
+		art = tb->symbols->vert;
+
+	return buffer_append_data(buf, art);
+}
+
 #define is_last_column(_tb, _cl) \
 		list_entry_is_last(&(_cl)->cl_columns, &(_tb)->tb_columns)
 
@@ -258,8 +284,33 @@
 		if (len > width && !scols_column_is_trunc(cl)) {
 			fputs(linesep(tb), tb->out);
 			for (i = 0; i <= (size_t) cl->seqnum; i++) {
-				struct libscols_column *x = scols_table_get_column(tb, i);
-				fprintf(tb->out, "%*s ", -((int)x->width), " ");
+				size_t len_pad = 0;	/* in screen cells as opposed to bytes */
+				struct libscols_column* cl_pad = scols_table_get_column(tb, i);
+				if(ln && scols_column_is_tree(cl_pad)) {
+					if (!ln->parent) {	/* Root cell needs special treatment. */
+						if (!list_empty(&ln->ln_branch)) {	/* only print symbols->vert if followed by something */
+							fputs(tb->symbols->vert, tb->out);
+							len_pad = mbs_safe_width(tb->symbols->vert);
+						}
+					} else {	/* use the same draw function as though we were intending to draw an L-shape */
+						struct libscols_buffer* b_art = new_buffer(1024);	/* TODO: FIXME proper size */
+						char* data = 0;
+
+						if(b_art) {
+							line_ascii_art_to_buffer(tb, ln, b_art);	/* whatever the rc, len_pad will be sensible */
+							data = buffer_get_safe_data(b_art, &len_pad);
+							if(data && len_pad) {
+								fputs(data, tb->out);
+							}
+
+							free_buffer(b_art);
+						}
+					}
+				}
+				for(; len_pad <= cl_pad->width; ++len_pad)
+				{
+					fputc(' ', tb->out);
+				}
 			}
 		} else
 			fputs(colsep(tb), tb->out);	/* columns separator */
@@ -268,32 +319,6 @@
 	return 0;
 }
 
-/* returns pointer to the end of used data */
-static int line_ascii_art_to_buffer(struct libscols_table *tb,
-				    struct libscols_line *ln,
-				    struct libscols_buffer *buf)
-{
-	const char *art;
-	int rc;
-
-	assert(ln);
-	assert(buf);
-
-	if (!ln->parent)
-		return 0;
-
-	rc = line_ascii_art_to_buffer(tb, ln->parent, buf);
-	if (rc)
-		return rc;
-
-	if (list_entry_is_last(&ln->ln_children, &ln->parent->ln_branch))
-		art = "  ";
-	else
-		art = tb->symbols->vert;
-
-	return buffer_append_data(buf, art);
-}
-
 static int cell_to_buffer(struct libscols_table *tb,
 			  struct libscols_line *ln,
 			  struct libscols_column *cl,
maintainer
#1 lore raw
Quoted 1 line · Phillip Susi

Forwarding patch from https://launchpad.net/bugs/1406133:

Applied with some changes, note that the patch does not resolve situation when the tree is not the first column, for example:

    $ findmnt -o SOURCE,FSTYPE,OPTIONS,TARGET

but it's fine. I have tried to implement it for for trees after the line break and then the output is pretty unreadable.

Karel

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com
#2 lore raw
In reply to Phillip Susi

src/table_print.c line 163

Quoted 5 lines · Phillip Susi
src/table_print.c +2 -0
 
+	assert(ln);
+	assert(buf);

general note: libraries should never assert/abort/exit. if this were internal, then you could make a case about the utilities being ok with that, but it looks like the intention is for people to include the header and link against it from outside of util-linux.
-mike

maintainer
#3 lore raw
Quoted 8 lines · Mike Frysinger

src/table_print.c line 163

Quoted 5 lines · Phillip Susi
src/table_print.c +2 -0
 
+	assert(ln);
+	assert(buf);

general note: libraries should never assert/abort/exit.

what about segfault?

We use assert to detect fatal internal library bugs, for mistakes in applications (e.g. incomplete arguments) it returns -EINVAL.

Its' possible to remove CONFIG_LIBSMARTCOLS_ASSERT from smartcolsP.h, but I don't think the library is already well tested.

Karel

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com
#4 lore raw
Quoted 11 lines · Karel Zak
Quoted 8 lines · Mike Frysinger

src/table_print.c line 163

Quoted 5 lines · Phillip Susi
src/table_print.c +2 -0
 
+	assert(ln);
+	assert(buf);

general note: libraries should never assert/abort/exit.

what about segfault?

that's a red herring. you can't guarantee your codebase is bug free, but that doesn't mean encouraging a policy of committing suicide is a good thing.

Quoted 2 lines

We use assert to detect fatal internal library bugs, for mistakes in applications (e.g. incomplete arguments) it returns -EINVAL.

libsmartcols/src/table_print.c:
int scols_print_table(struct libscols_table *tb)
{
...

    assert(tb);
    if (!tb)
        return -1;

looks like over-eagerness has already broken the public API ;).

Quoted 2 lines

Its' possible to remove CONFIG_LIBSMARTCOLS_ASSERT from smartcolsP.h, but I don't think the library is already well tested.

odd ... why is that reimplementing the wheel ? the standard assert.h already supports NDEBUG for turning off asserts.

i see there's a bunch of such knobs in the codebase. time for a --{en,dis}able-asserts configure flag ? you could delete all the existing CONFIG_xxx_ASSERT defines and simply rely on configure adding -DNDEBUG to CPPFLAGS.
-mike

maintainer
#5 lore raw
Quoted 6 lines · Mike Frysinger
Quoted 3 lines
Quoted 1 line

general note: libraries should never assert/abort/exit.

what about segfault?

that's a red herring. you can't guarantee your codebase is bug free, but that doesn't mean encouraging a policy of committing suicide is a good thing.

suicide + usable message is better than segfault

Quoted 10 lines
Quoted 2 lines

We use assert to detect fatal internal library bugs, for mistakes in applications (e.g. incomplete arguments) it returns -EINVAL.

libsmartcols/src/table_print.c:
int scols_print_table(struct libscols_table *tb)
{
...

    assert(tb);
    if (!tb)
        return -1;

this is mistake, fixed in libsmartcols and libmount, I'll later fix it in libfdisk too.

Quoted 10 lines
Quoted 2 lines

Its' possible to remove CONFIG_LIBSMARTCOLS_ASSERT from smartcolsP.h, but I don't think the library is already well tested.

odd ... why is that reimplementing the wheel ? the standard assert.h already supports NDEBUG for turning off asserts.

i see there's a bunch of such knobs in the codebase. time for a --{en,dis}able-asserts configure flag ? you could delete all the existing CONFIG_xxx_ASSERT defines and simply rely on configure adding -DNDEBUG to CPPFLAGS.

Maybe, the idea has been to keep it in our hands and force people to use asserts for the "not yet well tested" libs.

Karel

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com