libsmartcols patch

libsmartcols patch

[Phillip Susi]

[-- Attachment #1.1: Type: text/plain, Size: 61 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: libsmartcols-art-fix.diff --]
[-- Type: text/x-patch; name="libsmartcols-art-fix.diff", Size: 2987 bytes --]

--- libsmartcols/src/table_print.c	2014-12-24 23:59:55.780110296 +0200
+++ libsmartcols/src/table_print.c	2014-12-28 22:45:20.347285226 +0200
@@ -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,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: libsmartcols patch

[Karel Zak]

On Sun, Dec 28, 2014 at 08:30:00PM -0500, Phillip Susi wrote:
> 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

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: libsmartcols patch

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

On 28 Dec 2014 20:30, Phillip Susi wrote:
> --- libsmartcols/src/table_print.c	2014-12-24 23:59:55.780110296 +0200
> +++ libsmartcols/src/table_print.c	2014-12-28 22:45:20.347285226 +0200
>  
> +	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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: libsmartcols patch

[Karel Zak]

On Mon, Feb 16, 2015 at 12:00:53AM -0500, Mike Frysinger wrote:
> On 28 Dec 2014 20:30, Phillip Susi wrote:
> > --- libsmartcols/src/table_print.c	2014-12-24 23:59:55.780110296 +0200
> > +++ libsmartcols/src/table_print.c	2014-12-28 22:45:20.347285226 +0200
> >  
> > +	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

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: libsmartcols patch

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

On 16 Feb 2015 11:27, Karel Zak wrote:
> On Mon, Feb 16, 2015 at 12:00:53AM -0500, Mike Frysinger wrote:
> > On 28 Dec 2014 20:30, Phillip Susi wrote:
> > > --- libsmartcols/src/table_print.c	2014-12-24 23:59:55.780110296 +0200
> > > +++ libsmartcols/src/table_print.c	2014-12-28 22:45:20.347285226 +0200
> > >  
> > > +	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.

>  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 ;).

>  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: libsmartcols patch

[Karel Zak]

On Mon, Feb 16, 2015 at 05:48:48AM -0500, Mike Frysinger wrote:
> > > 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

> >  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.

> >  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

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com