On Mon, 2018-08-13 at 12:01 +0200, Michal Prívozník wrote:
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.
I see, I misunderstood. That's also an option.
+{ + 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.
I'm talking about getting rid of ctl also in printing to terminal.
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?
I could have it as an argument for vshPrintToTerminal and vshPrintToString (perhaps some bool whetever to print header or not, or some flags). And according to value of that bool, priting of header would or wouldn't commence. (Whole block of code which takes care of priting header would be in some if-statement or something). My point is that it would make whole table API priting more universal, so it could be called from place where there is no vshControl variable, and user still could "turn off" header even without vshControl variable. Therefore I'm suggesting to get rid of vshControl ctl variable, and for priting without header, let's use some bool or something. Simon.