[libvirt] [PATCH 0/9] virsh domfsinfo fixes
Fix a crash and other issues: https://bugzilla.redhat.com/show_bug.cgi?id=1676354 Ján Tomko (9): vshtabletest: indent strings with expected output vsh-table: allow empty columns virsh: introduce ninfos variable in cmdDomFSInfo virsh: rename ret to rc in cmdDomFSInfo virsh: do not access uninitialized memory in cmdDomFSInfo virsh: introduce 'ret' in cmdDomFSInfo virsh: do not report error on zero filesystems in cmdDomFSInfo virsh: use virBufferTrim in cmdDomFSInfo virsh: allow empty targets in cmdDomFSInfo tests/vshtabletest.c | 78 +++++++++++++++++++++++--------------------- tools/virsh-domain.c | 35 +++++++++++--------- tools/vsh-table.c | 3 -- 3 files changed, 60 insertions(+), 56 deletions(-) -- 2.19.2
Indent them by four spaces from the previous line, instead of starting at columnn zero. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/vshtabletest.c | 76 ++++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c index e795c5afc0..79285b14a9 100644 --- a/tests/vshtabletest.c +++ b/tests/vshtabletest.c @@ -43,13 +43,13 @@ testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED) int ret = 0; char *act = NULL; const char *exp = -" 1 fedora28 running\n" -" 2 rhel7.5 running\n"; + " 1 fedora28 running\n" + " 2 rhel7.5 running\n"; const char *exp2 = -" Id Name State\n" -"--------------------------\n" -" 1 fedora28 running\n" -" 2 rhel7.5 running\n"; + " Id Name State\n" + "--------------------------\n" + " 1 fedora28 running\n" + " 2 rhel7.5 running\n"; vshTablePtr table = vshTableNew("Id", "Name", "State", NULL); //to ask about return @@ -118,10 +118,10 @@ testUnicode(const void *opaque ATTRIBUTE_UNUSED) char *act = NULL; const char *exp = -" Id 名稱 государство\n" -"-----------------------------------------\n" -" 1 fedora28 running\n" -" 2 つへソrhel7.5つへソ running\n"; + " Id 名稱 государство\n" + "-----------------------------------------\n" + " 1 fedora28 running\n" + " 2 つへソrhel7.5つへソ running\n"; vshTablePtr table; table = vshTableNew("Id", "名稱", "государство", NULL); @@ -150,10 +150,10 @@ testUnicodeArabic(const void *opaque ATTRIBUTE_UNUSED) char *act = NULL; const char *exp = -" ﻡﺍ ﻢﻣﺍ ﻕﺎﺌﻣﺓ ﺓ ﺎﻠﺼﻋ ﺍﻸﺜﻧﺎﻧ\n" -"-------------------------------------------------------------------------------------------\n" -" 1 ﻉﺪﻴﻟ ﺎﻠﺜﻘﻴﻟ ﻕﺎﻣ ﻊﻧ, ٣٠ ﻎﻴﻨﻳﺍ ﻮﺘﻧﺎﻤﺗ ﺎﻠﺛﺎﻠﺛ، ﺄﺳﺭ, ﺩﻮﻟ ﺩﻮﻟ. ﺄﻣﺎﻣ ﺍ ﺎﻧ ﻲﻜﻧ\n" -" ﺺﻔﺣﺓ ﺖﻜﺘﻴﻛﺍً ﻊﻟ, ﺎﻠﺠﻧﻭﺩ ﻭﺎﻠﻌﺗﺍﺩ ﺵﺭ\n"; + " ﻡﺍ ﻢﻣﺍ ﻕﺎﺌﻣﺓ ﺓ ﺎﻠﺼﻋ ﺍﻸﺜﻧﺎﻧ\n" + "-------------------------------------------------------------------------------------------\n" + " 1 ﻉﺪﻴﻟ ﺎﻠﺜﻘﻴﻟ ﻕﺎﻣ ﻊﻧ, ٣٠ ﻎﻴﻨﻳﺍ ﻮﺘﻧﺎﻤﺗ ﺎﻠﺛﺎﻠﺛ، ﺄﺳﺭ, ﺩﻮﻟ ﺩﻮﻟ. ﺄﻣﺎﻣ ﺍ ﺎﻧ ﻲﻜﻧ\n" + " ﺺﻔﺣﺓ ﺖﻜﺘﻴﻛﺍً ﻊﻟ, ﺎﻠﺠﻧﻭﺩ ﻭﺎﻠﻌﺗﺍﺩ ﺵﺭ\n"; vshTablePtr table; wchar_t wc; @@ -192,10 +192,10 @@ testUnicodeZeroWidthChar(const void *opaque ATTRIBUTE_UNUSED) int ret = 0; vshTablePtr table = NULL; const char *exp = -" I\u200Bd Name \u200BStatus\n" -"--------------------------\n" -" 1\u200B fedora28 run\u200Bning\n" -" 2 rhel7.5 running\n"; + " I\u200Bd Name \u200BStatus\n" + "--------------------------\n" + " 1\u200B fedora28 run\u200Bning\n" + " 2 rhel7.5 running\n"; char *act = NULL; wchar_t wc; @@ -229,10 +229,10 @@ testUnicodeCombiningChar(const void *opaque ATTRIBUTE_UNUSED) int ret = 0; vshTablePtr table = NULL; const char *exp = -" Id Náme Ⓢtatus\n" -"--------------------------\n" -" 1 fědora28 running\n" -" 2 rhel running\n"; + " Id Náme Ⓢtatus\n" + "--------------------------\n" + " 1 fědora28 running\n" + " 2 rhel running\n"; char *act = NULL; table = vshTableNew("Id", "Náme", "Ⓢtatus", NULL); @@ -258,10 +258,10 @@ testUnicodeNonPrintableChar(const void *opaque ATTRIBUTE_UNUSED) int ret = 0; vshTablePtr table = NULL; const char *exp = -" I\\x09d Name Status\n" -"----------------------------------\n" -" 1 f\\x07edora28 running\n" -" 2 rhel7.5 running\n"; + " I\\x09d Name Status\n" + "----------------------------------\n" + " 1 f\\x07edora28 running\n" + " 2 rhel7.5 running\n"; char *act = NULL; table = vshTableNew("I\td", "Name", "Status", NULL); @@ -288,20 +288,20 @@ testNTables(const void *opaque ATTRIBUTE_UNUSED) vshTablePtr table2 = NULL; vshTablePtr table3 = NULL; const char *exp1 = -" Id Name Status\n" -"--------------------------\n" -" 1 fedora28 running\n" -" 2 rhel7.5 running\n"; + " Id Name Status\n" + "--------------------------\n" + " 1 fedora28 running\n" + " 2 rhel7.5 running\n"; const char *exp2 = -" Id Name Status\n" -"---------------------\n"; + " Id Name Status\n" + "---------------------\n"; const char *exp3 = -" Id\n" -"-----\n" -" 1\n" -" 2\n" -" 3\n" -" 4\n"; + " Id\n" + "-----\n" + " 1\n" + " 2\n" + " 3\n" + " 4\n"; char *act1 = NULL; char *act2 = NULL; char *act3 = NULL; -- 2.19.2
Trivially implement this by deleting the bogus check in vshTableSafeEncode. Now it returns an empty string for an empty string instead of returning NULL without setting an error. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/vshtabletest.c | 4 +++- tools/vsh-table.c | 3 --- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c index 79285b14a9..b07db3cf23 100644 --- a/tests/vshtabletest.c +++ b/tests/vshtabletest.c @@ -291,7 +291,8 @@ testNTables(const void *opaque ATTRIBUTE_UNUSED) " Id Name Status\n" "--------------------------\n" " 1 fedora28 running\n" - " 2 rhel7.5 running\n"; + " 2 rhel7.5 running\n" + " 3 gazpacho \n"; const char *exp2 = " Id Name Status\n" "---------------------\n"; @@ -311,6 +312,7 @@ testNTables(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; vshTableRowAppend(table1, "1", "fedora28", "running", NULL); vshTableRowAppend(table1, "2", "rhel7.5", "running", NULL); + vshTableRowAppend(table1, "3", "gazpacho", "", NULL); act1 = vshTablePrintToString(table1, true); table2 = vshTableNew("Id", "Name", "Status", NULL); diff --git a/tools/vsh-table.c b/tools/vsh-table.c index fda8f15879..8bd6d99778 100644 --- a/tools/vsh-table.c +++ b/tools/vsh-table.c @@ -219,9 +219,6 @@ vshTableSafeEncode(const char *s, size_t *width) memset(&st, 0, sizeof(st)); - if (!sz) - return NULL; - if (VIR_ALLOC_N(buf, (sz * HEX_ENCODE_LENGTH) + 1) < 0) return NULL; -- 2.19.2
Do not use 'ret' throughout the whole function to avoid confusion and comparison of unsigned 'i' against signed 'ret'. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8b20059335..630761e40e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13940,6 +13940,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) size_t i, j; virDomainFSInfoPtr *info; vshTablePtr table = NULL; + size_t ninfos = 0; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -13949,7 +13950,9 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("Unable to get filesystem information")); goto cleanup; } - if (ret == 0) { + ninfos = ret; + + if (ninfos == 0) { vshError(ctl, _("No filesystems are mounted in the domain")); goto cleanup; } @@ -13959,7 +13962,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) if (!table) goto cleanup; - for (i = 0; i < ret; i++) { + for (i = 0; i < ninfos; i++) { virBuffer targetsBuff = VIR_BUFFER_INITIALIZER; VIR_AUTOFREE(char *) targets = NULL; @@ -13985,7 +13988,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) cleanup: if (info) { - for (i = 0; i < ret; i++) + for (i = 0; i < ninfos; i++) virDomainFSInfoFree(info[i]); VIR_FREE(info); } -- 2.19.2
Leave the 'ret' variable for the current function's return value. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 630761e40e..651766cd84 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13936,7 +13936,7 @@ static bool cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; - int ret = -1; + int rc = -1; size_t i, j; virDomainFSInfoPtr *info; vshTablePtr table = NULL; @@ -13945,12 +13945,12 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; - ret = virDomainGetFSInfo(dom, &info, 0); - if (ret < 0) { + rc = virDomainGetFSInfo(dom, &info, 0); + if (rc < 0) { vshError(ctl, _("Unable to get filesystem information")); goto cleanup; } - ninfos = ret; + ninfos = rc; if (ninfos == 0) { vshError(ctl, _("No filesystems are mounted in the domain")); @@ -13994,7 +13994,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) } vshTableFree(table); virshDomainFree(dom); - return ret >= 0; + return rc >= 0; } const vshCmdDef domManagementCmds[] = { -- 2.19.2
Initialize 'info' to prevent accessing random access memory. Introduced by commit 3072ded released in 4.8.0. https://bugzilla.redhat.com/show_bug.cgi?id=1676354 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 651766cd84..d5026286c9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13938,7 +13938,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; int rc = -1; size_t i, j; - virDomainFSInfoPtr *info; + virDomainFSInfoPtr *info = NULL; vshTablePtr table = NULL; size_t ninfos = 0; -- 2.19.2
Failing to print the table is also a reason to return failure and print the reported error. Switch to the usual pattern where we fall through the cleanup label right after setting ret to true instead of infering the return value from the number of filesystems returned. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d5026286c9..cb2da8eb5b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13941,6 +13941,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) virDomainFSInfoPtr *info = NULL; vshTablePtr table = NULL; size_t ninfos = 0; + bool ret = false; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -13953,6 +13954,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) ninfos = rc; if (ninfos == 0) { + ret = true; vshError(ctl, _("No filesystems are mounted in the domain")); goto cleanup; } @@ -13986,6 +13988,8 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) vshTablePrintToStdout(table, ctl); } + ret = true; + cleanup: if (info) { for (i = 0; i < ninfos; i++) @@ -13994,7 +13998,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) } vshTableFree(table); virshDomainFree(dom); - return rc >= 0; + return ret; } const vshCmdDef domManagementCmds[] = { -- 2.19.2
Use vshPrintExtra to report this message. It is a human-readable explanation rather than an error. Also, it is a very special system that runs with no filesystems. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cb2da8eb5b..d21e77ce75 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13955,7 +13955,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) if (ninfos == 0) { ret = true; - vshError(ctl, _("No filesystems are mounted in the domain")); + vshPrintExtra(ctl, _("No filesystems are mounted in the domain")); goto cleanup; } -- 2.19.2
Add comma after every string and trim the final one. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d21e77ce75..6124126576 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13968,11 +13968,9 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) virBuffer targetsBuff = VIR_BUFFER_INITIALIZER; VIR_AUTOFREE(char *) targets = NULL; - for (j = 0; j < info[i]->ndevAlias; j++) { - virBufferAdd(&targetsBuff, info[i]->devAlias[j], -1); - if (j != info[i]->ndevAlias - 1) - virBufferAddChar(&targetsBuff, ','); - } + for (j = 0; j < info[i]->ndevAlias; j++) + virBufferAsprintf(&targetsBuff, "%s,", info[i]->devAlias[j]); + virBufferTrim(&targetsBuff, ",", -1); targets = virBufferContentAndReset(&targetsBuff); -- 2.19.2
Ever since the introduction of the guest-get-fsinfo command in QEMU commit 46d4c572 qga/qapi-schema.json says that the 'disks' array can possibly be empty. For example when getting the target list is unsupported: https://bugzilla.redhat.com/show_bug.cgi?id=1567041 Pass an empty string instead of NULL to vshTableRowAppend to prevent a mismatched column number. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6124126576..686fea8dd1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13978,7 +13978,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) info[i]->mountpoint, info[i]->name, info[i]->fstype, - targets, + targets ? : "", NULL) < 0) goto cleanup; } -- 2.19.2
On 2/12/19 10:21 AM, Ján Tomko wrote:
Ever since the introduction of the guest-get-fsinfo command in QEMU commit 46d4c572 qga/qapi-schema.json says that the 'disks' array can possibly be empty. For example when getting the target list is unsupported: https://bugzilla.redhat.com/show_bug.cgi?id=1567041
Pass an empty string instead of NULL to vshTableRowAppend to prevent a mismatched column number.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6124126576..686fea8dd1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13978,7 +13978,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) info[i]->mountpoint, info[i]->name, info[i]->fstype, - targets, + targets ? : "",
Last time I posted a patch that had this I was told (most probably by Eric) that this is not portable. I think we need to use expanded version.
NULL) < 0) goto cleanup; }
Michal
On Tue, Feb 12, 2019 at 01:17:25PM +0100, Michal Privoznik wrote:
On 2/12/19 10:21 AM, Ján Tomko wrote:
Ever since the introduction of the guest-get-fsinfo command in QEMU commit 46d4c572 qga/qapi-schema.json says that the 'disks' array can possibly be empty. For example when getting the target list is unsupported: https://bugzilla.redhat.com/show_bug.cgi?id=1567041
Pass an empty string instead of NULL to vshTableRowAppend to prevent a mismatched column number.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6124126576..686fea8dd1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13978,7 +13978,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) info[i]->mountpoint, info[i]->name, info[i]->fstype, - targets, + targets ? : "",
Last time I posted a patch that had this I was told (most probably by Eric) that this is not portable. I think we need to use expanded version.
There actually is some usage that sneaked into our code: ChangeLog-old: * src/remote_internal.c: Remove gcc-ism in empty "x ? : y" src/util/virdbus.c: localerror.message ? : _("unknown error")); src/vz/vz_sdk.c: pret = PrlVmCfg_SetVNCPassword(sdkdom, gr->data.vnc.auth.passwd ? : ""); src/vz/vz_sdk.c: pret = PrlVmDevNet_SetDefaultGateway(sdknet, gw4 ? : ""); src/vz/vz_sdk.c: pret = PrlVmDevNet_SetDefaultGatewayIPv6(sdknet, gw6 ? : ""); src/vz/vz_sdk.c: const char *path = disk->src->path ? : ""; src/vz/vz_sdk.c: description ? : ""); but I guess neither D-Bus nor vz are that portable. I have no problems using the portable version here. Jano
NULL) < 0) goto cleanup; }
Michal
On 2/12/19 10:21 AM, Ján Tomko wrote:
Fix a crash and other issues: https://bugzilla.redhat.com/show_bug.cgi?id=1676354
Ján Tomko (9): vshtabletest: indent strings with expected output vsh-table: allow empty columns virsh: introduce ninfos variable in cmdDomFSInfo virsh: rename ret to rc in cmdDomFSInfo virsh: do not access uninitialized memory in cmdDomFSInfo virsh: introduce 'ret' in cmdDomFSInfo virsh: do not report error on zero filesystems in cmdDomFSInfo virsh: use virBufferTrim in cmdDomFSInfo virsh: allow empty targets in cmdDomFSInfo
tests/vshtabletest.c | 78 +++++++++++++++++++++++--------------------- tools/virsh-domain.c | 35 +++++++++++--------- tools/vsh-table.c | 3 -- 3 files changed, 60 insertions(+), 56 deletions(-)
ACK Michal
participants (2)
-
Ján Tomko -
Michal Privoznik