Re: [libvirt] [PATCH 1/2] Add API for printing tables.

On 08/13/2018 11:46 AM, Simon Kobyda wrote: > On Fri, 2018-08-10 at 15:40 +0200, Michal Privoznik wrote: > > On 08/10/2018 11:11 AM, Simon Kobyda wrote: > > > +static vshTableRowPtr > > > +vshTableRowNew(size_t size, const char *arg, va_list ap) > > > > I know we discussed this in person, but now that I see this code > > and > > think about it more I think this @size argument should be dropped > > from > > this function and the corresponding check should be moved to [1] > > That way I will implement identical loop also there [1], to count > number of variable arguments to make that check (if there is better > way > to count number of variable arguments, I'm open to suggestions :) > ). > Sure I can drop it there, but what are pros of that? I'm curious > :). I'm not sure why you would need the second loop? The way your code works right now is that vshTableRowNew must have the sentinel (= args have to end with NULL). What I am suggesting is for vshTableRowNew() to just consume all the args (like it is doing now) until the sentinel. And only after that check at [1] whether row->ncells is expected value.

> > > > > +{ > > > + vshTableRowPtr row = NULL; > > > + char *tmp = NULL; > > > + > > > + if (!arg) { > > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > > > + _("Table row cannot be empty")); > > > + goto error; > > > + } > > > + > > > + if (VIR_ALLOC(row) < 0) > > > + goto error; > > > + > > > + while (arg) { > > > + if (VIR_STRDUP(tmp, arg) < 0) > > > + goto error; > > > + > > > + if (VIR_APPEND_ELEMENT(row->cells, row->ncells, tmp) < > > > 0) > > > + goto error; > > > + > > > + arg = va_arg(ap, const char *); > > > + } > > > + > > > + if (size && row->ncells != size) { > > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > > > + _("Incorrect number of cells in a > > > table > > > row")); > > > + goto error; > > > + } > > > + > > > + return row; > > > + > > > + error: > > > + vshTableRowFree(row); > > > + return NULL; > > > +} > > > + > > > + > > > +/** > > > + * vshTableRowPrint: > > > + * @ctl virtshell control structure > > > + * @row: table to append to > > > + * @maxwidths: maximum count of characters for each columns > > > + * @widths: count of character for each cell in this row > > > + * @printCB function for priting table > > > + * @buf: buffer to store table (only if @toStdout == true) > > > + * @toStdout: whetever print table to Stdout or return in > > > buffer > > > + */ > > > +static void > > > +vshTableRowPrint(vshControl *ctl, > > > + vshTableRowPtr row, > > > + size_t *maxwidths, > > > + size_t *widths, > > > + vshPrintCB printCB, > > > + virBufferPtr buf, > > > + bool toStdout) > > > +{ > > > + size_t i; > > > + size_t j; > > > + > > > + if (toStdout) { > > > + for (i = 0; i < row->ncells; i++) { > > > + printCB(ctl, " %s", row->cells[i]); > > > + > > > + for (j = 0; j < maxwidths[i] - widths[i] + 2; j++) > > > + printCB(ctl, " "); > > > + } > > > + printCB(ctl, "\n"); > > > + } else { > > > + for (i = 0; i < row->ncells; i++) { > > > + virBufferAsprintf(buf, " %s", row->cells[i]); > > > + > > > + for (j = 0; j < maxwidths[i] - widths[i] + 2; j++) > > > + virBufferAddStr(buf, " "); > > > + } > > > + virBufferAddStr(buf, "\n"); > > > + } > > > > > > How about inverting these ifs and fors? I mean: > > > > for () { > > if (toStdout) > > printCB() > > else > > virBufferAsprintf. > > } > > > > This suggestion applies to vshTablePrint too. > > I had it the way you are suggesting before, but I inverted it > because I > think the current state of code is more readable, compared to its > inverted state. > The length of code would be very similar, but perhaps it would be > more > optimized? (we would have only 2 'for' loops instead of 4, however > more > if-else statements) If that's the reason why are you suggesting to > invert it. Well, the reason is that if we ever need to change the way we're iterating (e.g. call some function at the end of each loop) we can do it in one change: for () { if (toStdout) printCB() else virBufferAsprintf() + newFunction(); } Michal > > > > > > + > > > +} > > > + > > > +/** > > > + * vshTablePrint: > > > + * @ctl virtshell control structure > > > + * @table: table to print > > > + * @toStdout: whetever to print to stdout (true) or return in > > > string (false) > > > + * > > > + * Prints table. To get an alignment of columns right, > > > function > > > + * fills 2d array @widths with count of characters in each > > > cell > > > and > > > + * array @maxwidths maximum count of character in each column. > > > + * Function then prints tables header and content. > > > + * > > > + * Returns string containing table, or NULL if table was > > > printed > > > to > > > + * stdout > > > + */ > > > +static char * > > > +vshTablePrint(vshControl *ctl, vshTablePtr table, bool > > > toStdout) > > > +{ > > > + size_t i; > > > + size_t j; > > > + size_t *maxwidths; > > > + size_t **widths; > > > + virBuffer buf = VIR_BUFFER_INITIALIZER; > > > + char *ret = NULL; > > > + > > > + if (VIR_ALLOC_N(maxwidths, table->rows[0]->ncells)) > > > + goto cleanup; > > > + > > > + if (VIR_ALLOC_N(widths, table->nrows)) > > > + goto cleanup; > > > + > > > + /* retrieve widths of columns */ > > > + for (i = 0; i < table->nrows; i++) { > > > + if (VIR_ALLOC_N(widths[i], table->rows[0]->ncells)) > > > + goto cleanup; > > > + } > > > + > > > + vshTableGetColumnsWidths(table, &maxwidths, &widths); > > > + > > > + /* print header */ > > > + VIR_WARNINGS_NO_PRINTF > > > + vshTableRowPrint(ctl, table->rows[0], maxwidths, > > > widths[0], > > > + vshPrintExtra, &buf, toStdout); > > > + VIR_WARNINGS_RESET > > > + > > > + /* print dividing line */ > > > + if (toStdout) { > > > + for (i = 0; i < table->rows[0]->ncells; i++) { > > > + for (j = 0; j < maxwidths[i] + 3; j++) > > > + vshPrintExtra(ctl, "-"); > > > + } > > > + vshPrintExtra(ctl, "\n"); > > > + } else { > > > + for (i = 0; i < table->rows[0]->ncells; i++) { > > > + for (j = 0; j < maxwidths[i] + 3; j++) > > > + virBufferAddStr(&buf, "-"); > > > + } > > > + virBufferAddStr(&buf, "\n"); > > > + } > > > + > > > + /* print content */ > > > + for (i = 1; i < table->nrows; i++) { > > > + VIR_WARNINGS_NO_PRINTF > > > + vshTableRowPrint(ctl, table->rows[i], maxwidths, > > > widths[0], > > > > Ooops. s/0/i/ ;-) > > > > > + vshPrintExtra, &buf, toStdout); > > > + VIR_WARNINGS_RESET > > > + } > > > + > > > + if (!toStdout) > > > + ret = virBufferContentAndReset(&buf); > > > + > > > + cleanup: > > > + VIR_FREE(maxwidths); > > > + for (i = 0; i < table->nrows; i++) > > > + VIR_FREE(widths[i]); > > > + VIR_FREE(widths); > > > + return ret; > > > +} > > > + > > > + > > > +void > > > +vshTablePrintToStdout(vshControl *ctl, vshTablePtr table) > > > +{ > > > + vshTablePrint(ctl, table, true); > > > +} > > > + > > > +char * > > > +vshTablePrintToString(vshControl *ctl, vshTablePtr table) > > > +{ > > > + return vshTablePrint(ctl, table, false); > > > +} > > > > Why does this function require @ctl? > > You are right its useless in vshTablePrintToString function. > > I'm also thinking about dropping ctl from this API completely, and > pass > NULL to vshPrintExtra, like vshPrintExtra(NULL, "something to > print"). I'm failing to see where would vshPrintExtra() be called with NULL. The point is that if you're printing to string virBuffer*() APIs are called.

> Would that have any further consequences? Yes, vshPrintExtra(NULL, ...) will print to stdout regardless of user requesting quiet output. For instance, `virsh -q list --all` doesn't print the table header, because it's printed with vshPrintExtra() which checks for '-q' and therefore doesn't print anything. However, that's not true if passed ctl is NULL. > My knowledge of ctl structure and its use in printing is shallow. > I know only that ctl function in priting has affect on --quiet > printing > (e.g. not priting table header), but that functionality could be > implemented other way. Does it have effect on anything else? > It would make API more universal, and quiet printing would not be > depended on ctl. I'm not quite sure how you want to achieve that. So currently when -q is passed, no table header is printed. If you don't pass that info to vshTablePrint*() then how is it supposed to know what to print?