[libvirt] [PATCH 1/2] Add API for printing tables.
Simon Kobyda
skobyda at redhat.com
Mon Aug 13 12:50:23 UTC 2018
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.
More information about the libvir-list
mailing list