Skip to content

Conversation

@ikegami-t
Copy link
Contributor

No description provided.

@ikegami-t
Copy link
Contributor Author

Changes for the issue #2912.

util/table.c Outdated
{
struct table *t = table_init();

if (t) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early return pattern is the preferred coding style, as it avoids deep indention levels:

if (!t)
   return NULL;

util/table.c Outdated
printf("%ld%s", val->ld, add_pad ? "" : " ");
else if (type == FMT_UNSIGNED_LONG)
printf("%lu", val->lu);
printf("%lu%s", val->lu, add_pad ? "" : " ");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would simpler to add a final printf for the padding. So just leave the above printfs as they are and after this block have something like:

if (add_pad)
  puts(" ");


default:
fprintf(stderr, "Invalid format!\n");
break;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here Just add a final padding printf if it is needed.

devname, genname, nvme_ns_get_serial(n),
nvme_ns_get_model(n), nvme_ns_get_nsid(n), usage, format,
nvme_ns_get_firmware(n));
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say when the table allocation fails just error out. no need for a fallback mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is not for the allocation error but for the zns list plugin command. The print function used by the both commands but currently the generic table is used by only the list command. (Later I will do chnage the zns list command also to use the generic table.)

@igaw
Copy link
Collaborator

igaw commented Oct 7, 2025

Looks good. I've tested quickly and for nvme list the output before and after are identically. The more interesting case is nvme list -v where the current fixed sized table is always a bit off. But great work! Thanks!

This combined table_init() and table_add_columns() functions.

Signed-off-by: Tokunori Ikegami <[email protected]>
The value auto populated but change it for the existing tables.

Signed-off-by: Tokunori Ikegami <[email protected]>
A space added between each columns but not necessary for end of line.

Signed-off-by: Tokunori Ikegami <[email protected]>
Use the generic table code added instead of hard coded table.

Signed-off-by: Tokunori Ikegami <[email protected]>
This changes as same with other switch case statements.

Signed-off-by: Tokunori Ikegami <[email protected]>
Delete the padding flag by padding in the caller functions.

Signed-off-by: Tokunori Ikegami <[email protected]>
@ikegami-t
Copy link
Contributor Author

Just fixed for the comments. Thank you.

util/table.c Outdated
struct table *table_init(void)
{
struct table *t;
struct table *t = malloc(sizeof(struct table));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could just be return calloc(1, sizeof(struct table));. Maybe we should drop the function as there is no real value in it and use calloc directly where currently table_init is used. Or do I miss something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see, table_free is doing some work and to keep the symetry table_init makes sense. But I think the name shouldbe table_create or so. It's not initializing anything, it's just allocation it.

util/table.c Outdated
}

return t;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary empy line.

Also changed the function to return calloc() instead.

Signed-off-by: Tokunori Ikegami <[email protected]>
@ikegami-t
Copy link
Contributor Author

Again fixed the review comments. Thank you!

@igaw igaw merged commit de1517d into linux-nvme:master Oct 8, 2025
15 checks passed
@igaw
Copy link
Collaborator

igaw commented Oct 8, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants