10:29 a.m.
The most important patch is the last one, the other two are just
cleanups I wrote while looking around.
Michal Prívozník (3):
qemu: Drop needless check in virDomainFSInfoFormat()
qemu: Move qemuAgentFSInfo array free into qemuDomainGetFSInfo()
qemu: Don't lie about @ndevAlias when translating FSInfo
src/qemu/qemu_driver.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
--
2.26.2
10:29 a.m.
New subject: [PATCH 1/3] qemu: Drop needless check in virDomainFSInfoFormat()
As the very first thing, this function checks whether the number
of items inside @agentinfo array is not negative. This is
redundant as the only caller - qemuDomainGetFSInfo() already
checked for that and would not even call this function if that
was the case.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3d54653217..f59f9e13ba 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18966,8 +18966,6 @@ virDomainFSInfoFormat(qemuAgentFSInfoPtr *agentinfo,
virDomainFSInfoPtr *info_ret = NULL;
size_t i;
- if (nagentinfo < 0)
- return ret;
info_ret = g_new0(virDomainFSInfoPtr, nagentinfo);
for (i = 0; i < nagentinfo; i++) {
--
2.26.2
10:54 a.m.
New subject: [PATCH 1/3] qemu: Drop needless check in virDomainFSInfoFormat()
On a Monday in 2021, Michal Privoznik wrote:
As the very first thing, this function checks whether the number
of items inside @agentinfo array is not negative. This is
redundant as the only caller - qemuDomainGetFSInfo() already
checked for that and would not even call this function if that
was the case.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 2 --
1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:29 a.m.
New subject: [PATCH 2/3] qemu: Move qemuAgentFSInfo array free into qemuDomainGetFSInfo()
When qemuDomainGetFSInfo() is called it calls
qemuDomainGetFSInfoAgent() which executes 'guest-get-fsinfo'
guest agent command, parses returned JSON and returns an array of
qemuAgentFSInfo structures (well, pointers to those structs).
Then it grabs a domain job and tries to do some matching of guest
returned info against domain definition. This matching is done in
virDomainFSInfoFormat() which also frees the array of
qemuAgentFSInfo structures allocated earlier.
But this is not just. If acquiring the domain job fails (or
domain activeness check executed right after that fails) then
virDomainFSInfoFormat() is not called, leaking the array of
structs.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f59f9e13ba..d30cf75b73 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18977,14 +18977,14 @@ virDomainFSInfoFormat(qemuAgentFSInfoPtr *agentinfo,
ret = nagentinfo;
cleanup:
- for (i = 0; i < nagentinfo; i++) {
- qemuAgentFSInfoFree(agentinfo[i]);
- /* if there was an error, free any memory we've allocated for the
- * return value */
- if (info_ret)
+ if (info_ret) {
+ for (i = 0; i < nagentinfo; i++) {
+ /* if there was an error, free any memory we've allocated for the
+ * return value */
virDomainFSInfoFree(info_ret[i]);
+ }
+ g_free(info_ret);
}
- g_free(info_ret);
return ret;
}
@@ -18997,7 +18997,7 @@ qemuDomainGetFSInfo(virDomainPtr dom,
virDomainObjPtr vm;
qemuAgentFSInfoPtr *agentinfo = NULL;
int ret = -1;
- int nfs;
+ int nfs = 0;
virCheckFlags(0, ret);
@@ -19022,7 +19022,12 @@ qemuDomainGetFSInfo(virDomainPtr dom,
qemuDomainObjEndJob(driver, vm);
cleanup:
- g_free(agentinfo);
+ if (agentinfo) {
+ size_t i;
+ for (i = 0; i < nfs; i++)
+ qemuAgentFSInfoFree(agentinfo[i]);
+ g_free(agentinfo);
+ }
virDomainObjEndAPI(&vm);
return ret;
}
--
2.26.2
11 a.m.
New subject: [PATCH 2/3] qemu: Move qemuAgentFSInfo array free into qemuDomainGetFSInfo()
On a Monday in 2021, Michal Privoznik wrote:
When qemuDomainGetFSInfo() is called it calls
qemuDomainGetFSInfoAgent() which executes 'guest-get-fsinfo'
guest agent command, parses returned JSON and returns an array of
qemuAgentFSInfo structures (well, pointers to those structs).
Then it grabs a domain job and tries to do some matching of guest
returned info against domain definition. This matching is done in
virDomainFSInfoFormat() which also frees the array of
qemuAgentFSInfo structures allocated earlier.
But this is not just. If acquiring the domain job fails (or
domain activeness check executed right after that fails) then
virDomainFSInfoFormat() is not called, leaking the array of
structs.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f59f9e13ba..d30cf75b73 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18977,14 +18977,14 @@ virDomainFSInfoFormat(qemuAgentFSInfoPtr *agentinfo,
ret = nagentinfo;
cleanup:
- for (i = 0; i < nagentinfo; i++) {
- qemuAgentFSInfoFree(agentinfo[i]);
- /* if there was an error, free any memory we've allocated for the
- * return value */
- if (info_ret)
+ if (info_ret) {
+ for (i = 0; i < nagentinfo; i++) {
+ /* if there was an error, free any memory we've allocated for the
+ * return value */
virDomainFSInfoFree(info_ret[i]);
+ }
+ g_free(info_ret);
}
- g_free(info_ret);
return ret;
}
This hunk is unrelated and just cosmetic.
@@ -18997,7 +18997,7 @@ qemuDomainGetFSInfo(virDomainPtr dom,
virDomainObjPtr vm;
qemuAgentFSInfoPtr *agentinfo = NULL;
int ret = -1;
- int nfs;
+ int nfs = 0;
virCheckFlags(0, ret);
@@ -19022,7 +19022,12 @@ qemuDomainGetFSInfo(virDomainPtr dom,
qemuDomainObjEndJob(driver, vm);
cleanup:
- g_free(agentinfo);
+ if (agentinfo) {
+ size_t i;
+ for (i = 0; i < nfs; i++)
+ qemuAgentFSInfoFree(agentinfo[i]);
+ g_free(agentinfo);
+ }
virDomainObjEndAPI(&vm);
return ret;
}
If split into two commits:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:58 a.m.
New subject: [PATCH 2/3] qemu: Move qemuAgentFSInfo array free into qemuDomainGetFSInfo()
On 2/15/21 6:00 PM, Ján Tomko wrote:
On a Monday in 2021, Michal Privoznik wrote:
> When qemuDomainGetFSInfo() is called it calls
> qemuDomainGetFSInfoAgent() which executes 'guest-get-fsinfo'
> guest agent command, parses returned JSON and returns an array of
> qemuAgentFSInfo structures (well, pointers to those structs).
> Then it grabs a domain job and tries to do some matching of guest
> returned info against domain definition. This matching is done in
> virDomainFSInfoFormat() which also frees the array of
> qemuAgentFSInfo structures allocated earlier.
>
> But this is not just. If acquiring the domain job fails (or
> domain activeness check executed right after that fails) then
> virDomainFSInfoFormat() is not called, leaking the array of
> structs.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f59f9e13ba..d30cf75b73 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18977,14 +18977,14 @@ virDomainFSInfoFormat(qemuAgentFSInfoPtr
> *agentinfo,
> ret = nagentinfo;
>
> cleanup:
> - for (i = 0; i < nagentinfo; i++) {
> - qemuAgentFSInfoFree(agentinfo[i]);
> - /* if there was an error, free any memory we've allocated for
> the
> - * return value */
> - if (info_ret)
> + if (info_ret) {
> + for (i = 0; i < nagentinfo; i++) {
> + /* if there was an error, free any memory we've allocated
> for the
> + * return value */
> virDomainFSInfoFree(info_ret[i]);
> + }
> + g_free(info_ret);
> }
> - g_free(info_ret);
> return ret;
> }
This hunk is unrelated and just cosmetic.
I'm not sure what you mean by 'unrelated'. The freeing must be moved in
one commit, otherwise either the array is leaked or double freed.
Michal
4:10 a.m.
New subject: [PATCH 2/3] qemu: Move qemuAgentFSInfo array free into qemuDomainGetFSInfo()
On a Monday in 2021, Michal Privoznik wrote:
On 2/15/21 6:00 PM, Ján Tomko wrote:
>On a Monday in 2021, Michal Privoznik wrote:
>>When qemuDomainGetFSInfo() is called it calls
>>qemuDomainGetFSInfoAgent() which executes 'guest-get-fsinfo'
>>guest agent command, parses returned JSON and returns an array of
>>qemuAgentFSInfo structures (well, pointers to those structs).
>>Then it grabs a domain job and tries to do some matching of guest
>>returned info against domain definition. This matching is done in
>>virDomainFSInfoFormat() which also frees the array of
>>qemuAgentFSInfo structures allocated earlier.
>>
>>But this is not just. If acquiring the domain job fails (or
>>domain activeness check executed right after that fails) then
>>virDomainFSInfoFormat() is not called, leaking the array of
>>structs.
>>
>>Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>---
>>src/qemu/qemu_driver.c | 21 +++++++++++++--------
>>1 file changed, 13 insertions(+), 8 deletions(-)
>>
>>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>index f59f9e13ba..d30cf75b73 100644
>>--- a/src/qemu/qemu_driver.c
>>+++ b/src/qemu/qemu_driver.c
>>@@ -18977,14 +18977,14 @@ virDomainFSInfoFormat(qemuAgentFSInfoPtr
>>*agentinfo,
>> ret = nagentinfo;
>>
>> cleanup:
>>- for (i = 0; i < nagentinfo; i++) {
>>- qemuAgentFSInfoFree(agentinfo[i]);
>>- /* if there was an error, free any memory we've allocated
>>for the
>>- * return value */
>>- if (info_ret)
>>+ if (info_ret) {
>>+ for (i = 0; i < nagentinfo; i++) {
>>+ /* if there was an error, free any memory we've
>>allocated for the
>>+ * return value */
>> virDomainFSInfoFree(info_ret[i]);
>>+ }
>>+ g_free(info_ret);
>> }
>>- g_free(info_ret);
>> return ret;
>>}
>
>This hunk is unrelated and just cosmetic.
I'm not sure what you mean by 'unrelated'. The freeing must be moved
in one commit, otherwise either the array is leaked or double freed.
Ah, now I see that little that little line hidden in there:
- qemuAgentFSInfoFree(agentinfo[i]);
I thought all it did was exchange the 'if' and 'for'.
Please leave that line in this commit and separate the exchange.
Jano
Michal
10:29 a.m.
New subject: [PATCH 3/3] qemu: Don't lie about @ndevAlias when translating FSInfo
When virDomainGetFSInfo() is called over a QEMU/KVM domain it
results into calling of 'guest-get-fsinfo' guest agent command to
which it replies with info on guest (mounted) filesystems. When
filling return structure we also try to do basic lookup and
translate guest agent provided disk address into disk target (as
seen in domain XML). This can of course fail - guest can have
variety of disks not recorded in domain XML (iSCSI, scsi_debug,
NFS to name a few). If that's the case, a debug message is logged
and no disk target is added into the return structure.
However, due to the way our code is written the caller is led to
believe that the target was added into the structure. This may
lead to a situation where the array of disk targets (strings)
contains NULL. But our RPC structure says the array contains only
non-NULL strings. This results in somewhat 'cryptic' (at least to
users) error message:
error: Unable to get filesystem information
error: Unable to encode message payload
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1919783
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d30cf75b73..0da9264b49 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18932,9 +18932,8 @@ qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent,
if (agent->disks)
ret->devAlias = g_new0(char *, agent->ndisks);
- ret->ndevAlias = agent->ndisks;
- for (i = 0; i < ret->ndevAlias; i++) {
+ for (i = 0; i < agent->ndisks; i++) {
qemuAgentDiskAddressPtr agentdisk = agent->disks[i];
virDomainDiskDefPtr diskDef;
@@ -18945,7 +18944,7 @@ qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent,
agentdisk->target,
agentdisk->unit);
if (diskDef != NULL)
- ret->devAlias[i] = g_strdup(diskDef->dst);
+ ret->devAlias[ret->ndevAlias++] = g_strdup(diskDef->dst);
else
VIR_DEBUG("Missing target name for '%s'.",
ret->mountpoint);
}
--
2.26.2
11:02 a.m.
New subject: [PATCH 3/3] qemu: Don't lie about @ndevAlias when translating FSInfo
On a Monday in 2021, Michal Privoznik wrote:
When virDomainGetFSInfo() is called over a QEMU/KVM domain it
results into calling of 'guest-get-fsinfo' guest agent command to
which it replies with info on guest (mounted) filesystems. When
filling return structure we also try to do basic lookup and
translate guest agent provided disk address into disk target (as
seen in domain XML). This can of course fail - guest can have
variety of disks not recorded in domain XML (iSCSI, scsi_debug,
NFS to name a few). If that's the case, a debug message is logged
and no disk target is added into the return structure.
However, due to the way our code is written the caller is led to
believe that the target was added into the structure. This may
lead to a situation where the array of disk targets (strings)
contains NULL. But our RPC structure says the array contains only
non-NULL strings. This results in somewhat 'cryptic' (at least to
users) error message:
error: Unable to get filesystem information
error: Unable to encode message payload
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1919783
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1378
days inactive
1379
days old
8 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik