Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
On Thu, Aug 02, 2018 at 09:23:05PM +0200, Matthias Bolte wrote:
> 2018-08-02 18:16 GMT+02:00 John Ferlan :
> >
> >
> > On 08/02/2018 12:11 PM, Marcos Paulo de Souza wrote:
> >> On Thu, Aug 02, 2018 at 05:37:46PM +0200, Matthias Bolte wrote:
> >>> 2018-08-02 16:45 GMT+02:00 John Ferlan :
>
>
> On 08/02/2018 10:07 AM, Matthias Bolte wrote:
> > 2018-08-02 15:20 GMT+02:00 John Ferlan :
> >>
> >>
> >> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
> >>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
> >>> :
> This is a new version from the last patchset sent yesterday, but now
> using
> VIR_STRNDUP, instead of allocating memory manually.
>
> First version:
> https://www.redhat.com/archives/libvir-list/2018-August/msg0.html
>
> Marcos Paulo de Souza (2):
> esx: Do not crash SetAutoStart by double free
> esx: Fix SetAutoStart invalid pointer free
>
> src/esx/esx_driver.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
> >>>
> >>> I see the problem, but your approach is too complicated.
> >>>
> >>> There is already code in place to handle those situations:
> >>>
> >>> 3417 cleanup:
> >>> 3418 if (newPowerInfo) {
> >>> 3419 newPowerInfo->key = NULL;
> >>> 3420 newPowerInfo->startAction = NULL;
> >>> 3421 newPowerInfo->stopAction = NULL;
> >>> 3422 }
> >>>
> >>> That resets those fields to NULL to avoid double freeing and freeing
> >>> static strings.
> >>>
> >>> The problem is that commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5 by
> >>> John Frelan broke this logic, by setting newPowerInfo to NULL in the
> >>> success path, to silence Coverity.
> >>>
> >>
> >> Memory recall from "Tue Sep 3 07:20:34 2013 -0400" is perhaps
> >> impossible
> >> ;-)... TL;DR, looks like the error back then was a false positive
> >> because (as I know I've learned since then) Coverity has a hard time
> >> when a boolean or size_t count is used to control whether or not memory
> >> would be freed.
> >>
> >> Looking at the logic:
> >>
> >> if (esxVI_AutoStartPowerInfo_AppendToList(>powerInfo,
> >> newPowerInfo) < 0) {
> >> goto cleanup;
> >> }
> >>
> >> newPowerInfo = NULL;
> >>
> >> and following it to esxVI_List_Append which on success would seemingly
> >> assign @newPowerInfo into the >powerInfo list, I can certainly
> >> see
> >> why logically setting newPowerInfo = NULL after than would seem to be
> >> the right thing. Of course, I see now the subtle reason why it's not a
> >> good idea.
> >>
> >> Restoring logic from that commit in esxDomainSetAutostart, then
> >> Coverity
> >> believes that @newPowerInfo is leaked from the goto cleanup at
> >> allocation because for some reason it has decided it must evaluate both
> >> true and false of a condition even though the logic only ever set the
> >> boolean when @newPowerInfo was placed onto the @spec->powerInfo list.
> >> IOW: A false positive because the human can read the code and say that
> >> Coverity is full of it.
> >>
> >> So either I add this to my Coverity list of false positives or in the
> >> "if (esxVI_AutoStartPowerInfo_Alloc() < 0 || " condition
> >> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
> >> to cleanup, removing it from the cleanup: path, and then remove the
> >> "newPowerInfo = NULL;" after list insertion.
> >>
> >>
> >>
> >> John
> >
> > Okay, I see what's going on. I suggest this alternative patch that
> > keeps the newPowerInfo = NULL line to make Coverity understand the
> > cleanup code, but adds a second variable to restore the original
> > logic. I hope this doesn't upset Coverity.
> >
> > Marcos, can you give the attached patch a try? It should fix the
> > problems you try to fix here, by restoring the original cleanup logic.
> >
>
> That patch was confusing at best... The following works just fine:
>
> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
> index cee98ebcaf..a3982089e3 100644
> --- a/src/esx/esx_driver.c
> +++ b/src/esx/esx_driver.c
> @@ -3387,6 +3387,8 @@ esxDomainSetAutostart(virDomainPtr domain, int
> autostart)
> esxVI_Int_Alloc(>startOrder) < 0 ||
> esxVI_Int_Alloc(>startDelay) < 0 ||
> esxVI_Int_Alloc(>stopDelay) < 0) {
> +
> +esxVI_AutoStartPowerInfo_Free();
> goto cleanup;
> }
>
> @@ -3403,8 +3405,6 @@ esxDomainSetAutostart(virDomainPtr domain, int
> autostart)
> goto cleanup;
> }
>
[libvirt] [PATCH 1/5] virauth.c: Check for valid auth callback
Instead of adding the same check for every drivers, execute the checks
in virAuthGetUsername and virAuthGetPassword. These funtions are called
when user is not set in the URI.
Signed-off-by: Marcos Paulo de Souza
---
src/util/virauth.c | 12
1 file changed, 12 insertions(+)
diff --git a/src/util/virauth.c b/src/util/virauth.c
index 8c450b6b31..759b8f0cd3 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -198,6 +198,12 @@ virAuthGetUsername(virConnectPtr conn,
if (virAuthGetConfigFilePath(conn, ) < 0)
return NULL;
+if (!auth || !auth->cb) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("Missing or invalid auth pointer"));
+return NULL;
+}
+
return virAuthGetUsernamePath(path, auth, servicename,
defaultUsername, hostname);
}
@@ -262,5 +268,11 @@ virAuthGetPassword(virConnectPtr conn,
if (virAuthGetConfigFilePath(conn, ) < 0)
return NULL;
+if (!auth || !auth->cb) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("Missing or invalid auth pointer"));
+return NULL;
+}
+
return virAuthGetPasswordPath(path, auth, servicename, username, hostname);
}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] esx: Drop check for auth and auth->cb
Since they are done inside virAuthGetPassword and virAuthGetUsername
when needed.
Signed-off-by: Marcos Paulo de Souza
---
src/esx/esx_driver.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index c2154799fa..15785858c6 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -854,13 +854,6 @@ esxConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
conn->uri->path, conn->uri->scheme);
}
-/* Require auth */
-if (!auth || !auth->cb) {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("Missing or invalid auth pointer"));
-return VIR_DRV_OPEN_ERROR;
-}
-
/* Allocate per-connection private data */
if (VIR_ALLOC(priv) < 0)
goto cleanup;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] xenapi: Drop check for auth
Since they are done inside virAuthGetPassword and virAuthGetUsername
when needed. Also, only auth is checked, but auth->cb, which that could
lead to a crash if the callback is NULL.
Signed-off-by: Marcos Paulo de Souza
---
src/xenapi/xenapi_driver.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c
index 34f9e2c717..3af5eeafcf 100644
--- a/src/xenapi/xenapi_driver.c
+++ b/src/xenapi/xenapi_driver.c
@@ -152,12 +152,6 @@ xenapiConnectOpen(virConnectPtr conn, virConnectAuthPtr
auth,
goto error;
}
-if (auth == NULL) {
-xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED,
- _("Authentication Credentials not found"));
-goto error;
-}
-
if (conn->uri->user != NULL) {
if (VIR_STRDUP(username, conn->uri->user) < 0)
goto error;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
On 08/02/2018 12:11 PM, Marcos Paulo de Souza wrote:
> On Thu, Aug 02, 2018 at 05:37:46PM +0200, Matthias Bolte wrote:
>> 2018-08-02 16:45 GMT+02:00 John Ferlan :
>>>
>>>
>>> On 08/02/2018 10:07 AM, Matthias Bolte wrote:
2018-08-02 15:20 GMT+02:00 John Ferlan :
>
>
> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
>> :
>>> This is a new version from the last patchset sent yesterday, but now
>>> using
>>> VIR_STRNDUP, instead of allocating memory manually.
>>>
>>> First version:
>>> https://www.redhat.com/archives/libvir-list/2018-August/msg0.html
>>>
>>> Marcos Paulo de Souza (2):
>>> esx: Do not crash SetAutoStart by double free
>>> esx: Fix SetAutoStart invalid pointer free
>>>
>>> src/esx/esx_driver.c | 14 +++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> I see the problem, but your approach is too complicated.
>>
>> There is already code in place to handle those situations:
>>
>> 3417 cleanup:
>> 3418 if (newPowerInfo) {
>> 3419 newPowerInfo->key = NULL;
>> 3420 newPowerInfo->startAction = NULL;
>> 3421 newPowerInfo->stopAction = NULL;
>> 3422 }
>>
>> That resets those fields to NULL to avoid double freeing and freeing
>> static strings.
>>
>> The problem is that commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5 by
>> John Frelan broke this logic, by setting newPowerInfo to NULL in the
>> success path, to silence Coverity.
>>
>
> Memory recall from "Tue Sep 3 07:20:34 2013 -0400" is perhaps impossible
> ;-)... TL;DR, looks like the error back then was a false positive
> because (as I know I've learned since then) Coverity has a hard time
> when a boolean or size_t count is used to control whether or not memory
> would be freed.
>
> Looking at the logic:
>
> if (esxVI_AutoStartPowerInfo_AppendToList(>powerInfo,
> newPowerInfo) < 0) {
> goto cleanup;
> }
>
> newPowerInfo = NULL;
>
> and following it to esxVI_List_Append which on success would seemingly
> assign @newPowerInfo into the >powerInfo list, I can certainly see
> why logically setting newPowerInfo = NULL after than would seem to be
> the right thing. Of course, I see now the subtle reason why it's not a
> good idea.
>
> Restoring logic from that commit in esxDomainSetAutostart, then Coverity
> believes that @newPowerInfo is leaked from the goto cleanup at
> allocation because for some reason it has decided it must evaluate both
> true and false of a condition even though the logic only ever set the
> boolean when @newPowerInfo was placed onto the @spec->powerInfo list.
> IOW: A false positive because the human can read the code and say that
> Coverity is full of it.
>
> So either I add this to my Coverity list of false positives or in the
> "if (esxVI_AutoStartPowerInfo_Alloc() < 0 || " condition
> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
> to cleanup, removing it from the cleanup: path, and then remove the
> "newPowerInfo = NULL;" after list insertion.
>
>
>
> John
Okay, I see what's going on. I suggest this alternative patch that
keeps the newPowerInfo = NULL line to make Coverity understand the
cleanup code, but adds a second variable to restore the original
logic. I hope this doesn't upset Coverity.
Marcos, can you give the attached patch a try? It should fix the
problems you try to fix here, by restoring the original cleanup logic.
>>>
>>> That patch was confusing at best... The following works just fine:
>>>
>>> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
>>> index cee98ebcaf..a3982089e3 100644
>>> --- a/src/esx/esx_driver.c
>>> +++ b/src/esx/esx_driver.c
>>> @@ -3387,6 +3387,8 @@ esxDomainSetAutostart(virDomainPtr domain, int
>>> autostart)
>>> esxVI_Int_Alloc(>startOrder) < 0 ||
>>> esxVI_Int_Alloc(>startDelay) < 0 ||
>>> esxVI_Int_Alloc(>stopDelay) < 0) {
>>> +
>>> +esxVI_AutoStartPowerInfo_Free();
>>> goto cleanup;
>>> }
>>>
>>> @@ -3403,8 +3405,6 @@ esxDomainSetAutostart(virDomainPtr domain, int
>>> autostart)
>>> goto cleanup;
>>> }
>>>
>>> -newPowerInfo = NULL;
>>> -
>>> if (esxVI_ReconfigureAutostart
>>>(priv->primary,
>>> priv->primary->hostSystem->configManager->autoStartManager,
>>> @@ -3426,8 +3426,6 @@ esxDomainSetAutostart(virDomainPtr domain, int
>>> autostart)
>>> esxVI_AutoStartDefaults_Free();
>>> esxVI_AutoStartPowerInfo_Free();
>>>
>>> -esxVI_AutoStartPowerInfo_Free();
>>> -
>>> return result;
>>> }
>>>
>>>
>>> A comment could be added that
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
On Thu, Aug 02, 2018 at 05:37:46PM +0200, Matthias Bolte wrote:
> 2018-08-02 16:45 GMT+02:00 John Ferlan :
> >
> >
> > On 08/02/2018 10:07 AM, Matthias Bolte wrote:
> >> 2018-08-02 15:20 GMT+02:00 John Ferlan :
> >>>
> >>>
> >>> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
> :
> > This is a new version from the last patchset sent yesterday, but now
> > using
> > VIR_STRNDUP, instead of allocating memory manually.
> >
> > First version:
> > https://www.redhat.com/archives/libvir-list/2018-August/msg0.html
> >
> > Marcos Paulo de Souza (2):
> > esx: Do not crash SetAutoStart by double free
> > esx: Fix SetAutoStart invalid pointer free
> >
> > src/esx/esx_driver.c | 14 +++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
>
> I see the problem, but your approach is too complicated.
>
> There is already code in place to handle those situations:
>
> 3417 cleanup:
> 3418 if (newPowerInfo) {
> 3419 newPowerInfo->key = NULL;
> 3420 newPowerInfo->startAction = NULL;
> 3421 newPowerInfo->stopAction = NULL;
> 3422 }
>
> That resets those fields to NULL to avoid double freeing and freeing
> static strings.
>
> The problem is that commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5 by
> John Frelan broke this logic, by setting newPowerInfo to NULL in the
> success path, to silence Coverity.
>
> >>>
> >>> Memory recall from "Tue Sep 3 07:20:34 2013 -0400" is perhaps impossible
> >>> ;-)... TL;DR, looks like the error back then was a false positive
> >>> because (as I know I've learned since then) Coverity has a hard time
> >>> when a boolean or size_t count is used to control whether or not memory
> >>> would be freed.
> >>>
> >>> Looking at the logic:
> >>>
> >>> if (esxVI_AutoStartPowerInfo_AppendToList(>powerInfo,
> >>> newPowerInfo) < 0) {
> >>> goto cleanup;
> >>> }
> >>>
> >>> newPowerInfo = NULL;
> >>>
> >>> and following it to esxVI_List_Append which on success would seemingly
> >>> assign @newPowerInfo into the >powerInfo list, I can certainly see
> >>> why logically setting newPowerInfo = NULL after than would seem to be
> >>> the right thing. Of course, I see now the subtle reason why it's not a
> >>> good idea.
> >>>
> >>> Restoring logic from that commit in esxDomainSetAutostart, then Coverity
> >>> believes that @newPowerInfo is leaked from the goto cleanup at
> >>> allocation because for some reason it has decided it must evaluate both
> >>> true and false of a condition even though the logic only ever set the
> >>> boolean when @newPowerInfo was placed onto the @spec->powerInfo list.
> >>> IOW: A false positive because the human can read the code and say that
> >>> Coverity is full of it.
> >>>
> >>> So either I add this to my Coverity list of false positives or in the
> >>> "if (esxVI_AutoStartPowerInfo_Alloc() < 0 || " condition
> >>> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
> >>> to cleanup, removing it from the cleanup: path, and then remove the
> >>> "newPowerInfo = NULL;" after list insertion.
> >>>
> >>>
> >>>
> >>> John
> >>
> >> Okay, I see what's going on. I suggest this alternative patch that
> >> keeps the newPowerInfo = NULL line to make Coverity understand the
> >> cleanup code, but adds a second variable to restore the original
> >> logic. I hope this doesn't upset Coverity.
> >>
> >> Marcos, can you give the attached patch a try? It should fix the
> >> problems you try to fix here, by restoring the original cleanup logic.
> >>
> >
> > That patch was confusing at best... The following works just fine:
> >
> > diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
> > index cee98ebcaf..a3982089e3 100644
> > --- a/src/esx/esx_driver.c
> > +++ b/src/esx/esx_driver.c
> > @@ -3387,6 +3387,8 @@ esxDomainSetAutostart(virDomainPtr domain, int
> > autostart)
> > esxVI_Int_Alloc(>startOrder) < 0 ||
> > esxVI_Int_Alloc(>startDelay) < 0 ||
> > esxVI_Int_Alloc(>stopDelay) < 0) {
> > +
> > +esxVI_AutoStartPowerInfo_Free();
> > goto cleanup;
> > }
> >
> > @@ -3403,8 +3405,6 @@ esxDomainSetAutostart(virDomainPtr domain, int
> > autostart)
> > goto cleanup;
> > }
> >
> > -newPowerInfo = NULL;
> > -
> > if (esxVI_ReconfigureAutostart
> >(priv->primary,
> > priv->primary->hostSystem->configManager->autoStartManager,
> > @@ -3426,8 +3426,6 @@ esxDomainSetAutostart(virDomainPtr domain, int
> > autostart)
> > esxVI_AutoStartDefaults_Free();
> > esxVI_AutoStartPowerInfo_Free();
> >
> > -esxVI_AutoStartPowerInfo_Free();
> > -
> > return result;
> > }
> >
> >
> > A comment could be added that indicates by moving the *_Free to cleanup:
> > along with
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
2018-08-02 16:45 GMT+02:00 John Ferlan :
>
>
> On 08/02/2018 10:07 AM, Matthias Bolte wrote:
>> 2018-08-02 15:20 GMT+02:00 John Ferlan :
>>>
>>>
>>> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
:
> This is a new version from the last patchset sent yesterday, but now using
> VIR_STRNDUP, instead of allocating memory manually.
>
> First version:
> https://www.redhat.com/archives/libvir-list/2018-August/msg0.html
>
> Marcos Paulo de Souza (2):
> esx: Do not crash SetAutoStart by double free
> esx: Fix SetAutoStart invalid pointer free
>
> src/esx/esx_driver.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
I see the problem, but your approach is too complicated.
There is already code in place to handle those situations:
3417 cleanup:
3418 if (newPowerInfo) {
3419 newPowerInfo->key = NULL;
3420 newPowerInfo->startAction = NULL;
3421 newPowerInfo->stopAction = NULL;
3422 }
That resets those fields to NULL to avoid double freeing and freeing
static strings.
The problem is that commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5 by
John Frelan broke this logic, by setting newPowerInfo to NULL in the
success path, to silence Coverity.
>>>
>>> Memory recall from "Tue Sep 3 07:20:34 2013 -0400" is perhaps impossible
>>> ;-)... TL;DR, looks like the error back then was a false positive
>>> because (as I know I've learned since then) Coverity has a hard time
>>> when a boolean or size_t count is used to control whether or not memory
>>> would be freed.
>>>
>>> Looking at the logic:
>>>
>>> if (esxVI_AutoStartPowerInfo_AppendToList(>powerInfo,
>>> newPowerInfo) < 0) {
>>> goto cleanup;
>>> }
>>>
>>> newPowerInfo = NULL;
>>>
>>> and following it to esxVI_List_Append which on success would seemingly
>>> assign @newPowerInfo into the >powerInfo list, I can certainly see
>>> why logically setting newPowerInfo = NULL after than would seem to be
>>> the right thing. Of course, I see now the subtle reason why it's not a
>>> good idea.
>>>
>>> Restoring logic from that commit in esxDomainSetAutostart, then Coverity
>>> believes that @newPowerInfo is leaked from the goto cleanup at
>>> allocation because for some reason it has decided it must evaluate both
>>> true and false of a condition even though the logic only ever set the
>>> boolean when @newPowerInfo was placed onto the @spec->powerInfo list.
>>> IOW: A false positive because the human can read the code and say that
>>> Coverity is full of it.
>>>
>>> So either I add this to my Coverity list of false positives or in the
>>> "if (esxVI_AutoStartPowerInfo_Alloc() < 0 || " condition
>>> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
>>> to cleanup, removing it from the cleanup: path, and then remove the
>>> "newPowerInfo = NULL;" after list insertion.
>>>
>>>
>>>
>>> John
>>
>> Okay, I see what's going on. I suggest this alternative patch that
>> keeps the newPowerInfo = NULL line to make Coverity understand the
>> cleanup code, but adds a second variable to restore the original
>> logic. I hope this doesn't upset Coverity.
>>
>> Marcos, can you give the attached patch a try? It should fix the
>> problems you try to fix here, by restoring the original cleanup logic.
>>
>
> That patch was confusing at best... The following works just fine:
>
> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
> index cee98ebcaf..a3982089e3 100644
> --- a/src/esx/esx_driver.c
> +++ b/src/esx/esx_driver.c
> @@ -3387,6 +3387,8 @@ esxDomainSetAutostart(virDomainPtr domain, int
> autostart)
> esxVI_Int_Alloc(>startOrder) < 0 ||
> esxVI_Int_Alloc(>startDelay) < 0 ||
> esxVI_Int_Alloc(>stopDelay) < 0) {
> +
> +esxVI_AutoStartPowerInfo_Free();
> goto cleanup;
> }
>
> @@ -3403,8 +3405,6 @@ esxDomainSetAutostart(virDomainPtr domain, int
> autostart)
> goto cleanup;
> }
>
> -newPowerInfo = NULL;
> -
> if (esxVI_ReconfigureAutostart
>(priv->primary,
> priv->primary->hostSystem->configManager->autoStartManager,
> @@ -3426,8 +3426,6 @@ esxDomainSetAutostart(virDomainPtr domain, int
> autostart)
> esxVI_AutoStartDefaults_Free();
> esxVI_AutoStartPowerInfo_Free();
>
> -esxVI_AutoStartPowerInfo_Free();
> -
> return result;
> }
>
>
> A comment could be added that indicates by moving the *_Free to cleanup:
> along with the setting of newPowerInfo = NULL after the AppendToList
> caused issues with eventual esxVI_HostAutoStartManagerConfig_Free trying
> to VIR_FREE static strings and double freeing the machine object.
>
> John
Your approach works, but it doesn't handle the
esxVI_AutoStartPowerInfo_AppendToList cleanup case in which
[libvirt] [PATCH] qemu_migration: Avoid writing to freed memory
When a domain is killed on the source host while it is being migrated
and libvirtd is waiting for the migration to finish (waiting for the
domain condition in qemuMigrationSrcWaitForCompletion), the run-time
state including priv->job.current may already be freed once
virDomainObjWait returns with -1. Thus the priv->job.current pointer
cached in jobInfo is no longer valid and setting jobInfo->status may
crash the daemon.
https://bugzilla.redhat.com/show_bug.cgi?id=1593137
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_migration.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 435cd174af..825a9d399b 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1584,7 +1584,8 @@ qemuMigrationSrcWaitForCompletion(virQEMUDriverPtr driver,
if (events) {
if (virDomainObjWait(vm) < 0) {
-jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
+if (virDomainObjIsActive(vm))
+jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
return -2;
}
} else {
--
2.18.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
On 08/02/2018 10:07 AM, Matthias Bolte wrote:
> 2018-08-02 15:20 GMT+02:00 John Ferlan :
>>
>>
>> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
>>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
>>> :
This is a new version from the last patchset sent yesterday, but now using
VIR_STRNDUP, instead of allocating memory manually.
First version:
https://www.redhat.com/archives/libvir-list/2018-August/msg0.html
Marcos Paulo de Souza (2):
esx: Do not crash SetAutoStart by double free
esx: Fix SetAutoStart invalid pointer free
src/esx/esx_driver.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> I see the problem, but your approach is too complicated.
>>>
>>> There is already code in place to handle those situations:
>>>
>>> 3417 cleanup:
>>> 3418 if (newPowerInfo) {
>>> 3419 newPowerInfo->key = NULL;
>>> 3420 newPowerInfo->startAction = NULL;
>>> 3421 newPowerInfo->stopAction = NULL;
>>> 3422 }
>>>
>>> That resets those fields to NULL to avoid double freeing and freeing
>>> static strings.
>>>
>>> The problem is that commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5 by
>>> John Frelan broke this logic, by setting newPowerInfo to NULL in the
>>> success path, to silence Coverity.
>>>
>>
>> Memory recall from "Tue Sep 3 07:20:34 2013 -0400" is perhaps impossible
>> ;-)... TL;DR, looks like the error back then was a false positive
>> because (as I know I've learned since then) Coverity has a hard time
>> when a boolean or size_t count is used to control whether or not memory
>> would be freed.
>>
>> Looking at the logic:
>>
>> if (esxVI_AutoStartPowerInfo_AppendToList(>powerInfo,
>> newPowerInfo) < 0) {
>> goto cleanup;
>> }
>>
>> newPowerInfo = NULL;
>>
>> and following it to esxVI_List_Append which on success would seemingly
>> assign @newPowerInfo into the >powerInfo list, I can certainly see
>> why logically setting newPowerInfo = NULL after than would seem to be
>> the right thing. Of course, I see now the subtle reason why it's not a
>> good idea.
>>
>> Restoring logic from that commit in esxDomainSetAutostart, then Coverity
>> believes that @newPowerInfo is leaked from the goto cleanup at
>> allocation because for some reason it has decided it must evaluate both
>> true and false of a condition even though the logic only ever set the
>> boolean when @newPowerInfo was placed onto the @spec->powerInfo list.
>> IOW: A false positive because the human can read the code and say that
>> Coverity is full of it.
>>
>> So either I add this to my Coverity list of false positives or in the
>> "if (esxVI_AutoStartPowerInfo_Alloc() < 0 || " condition
>> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
>> to cleanup, removing it from the cleanup: path, and then remove the
>> "newPowerInfo = NULL;" after list insertion.
>>
>>
>>
>> John
>
> Okay, I see what's going on. I suggest this alternative patch that
> keeps the newPowerInfo = NULL line to make Coverity understand the
> cleanup code, but adds a second variable to restore the original
> logic. I hope this doesn't upset Coverity.
>
> Marcos, can you give the attached patch a try? It should fix the
> problems you try to fix here, by restoring the original cleanup logic.
>
That patch was confusing at best... The following works just fine:
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index cee98ebcaf..a3982089e3 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -3387,6 +3387,8 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
esxVI_Int_Alloc(>startOrder) < 0 ||
esxVI_Int_Alloc(>startDelay) < 0 ||
esxVI_Int_Alloc(>stopDelay) < 0) {
+
+esxVI_AutoStartPowerInfo_Free();
goto cleanup;
}
@@ -3403,8 +3405,6 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
goto cleanup;
}
-newPowerInfo = NULL;
-
if (esxVI_ReconfigureAutostart
(priv->primary,
priv->primary->hostSystem->configManager->autoStartManager,
@@ -3426,8 +3426,6 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
esxVI_AutoStartDefaults_Free();
esxVI_AutoStartPowerInfo_Free();
-esxVI_AutoStartPowerInfo_Free();
-
return result;
}
A comment could be added that indicates by moving the *_Free to cleanup:
along with the setting of newPowerInfo = NULL after the AppendToList
caused issues with eventual esxVI_HostAutoStartManagerConfig_Free trying
to VIR_FREE static strings and double freeing the machine object.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
2018-08-02 15:20 GMT+02:00 John Ferlan :
>
>
> On 08/02/2018 05:04 AM, Matthias Bolte wrote:
>> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza
>> :
>>> This is a new version from the last patchset sent yesterday, but now using
>>> VIR_STRNDUP, instead of allocating memory manually.
>>>
>>> First version:
>>> https://www.redhat.com/archives/libvir-list/2018-August/msg0.html
>>>
>>> Marcos Paulo de Souza (2):
>>> esx: Do not crash SetAutoStart by double free
>>> esx: Fix SetAutoStart invalid pointer free
>>>
>>> src/esx/esx_driver.c | 14 +++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> I see the problem, but your approach is too complicated.
>>
>> There is already code in place to handle those situations:
>>
>> 3417 cleanup:
>> 3418 if (newPowerInfo) {
>> 3419 newPowerInfo->key = NULL;
>> 3420 newPowerInfo->startAction = NULL;
>> 3421 newPowerInfo->stopAction = NULL;
>> 3422 }
>>
>> That resets those fields to NULL to avoid double freeing and freeing
>> static strings.
>>
>> The problem is that commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5 by
>> John Frelan broke this logic, by setting newPowerInfo to NULL in the
>> success path, to silence Coverity.
>>
>
> Memory recall from "Tue Sep 3 07:20:34 2013 -0400" is perhaps impossible
> ;-)... TL;DR, looks like the error back then was a false positive
> because (as I know I've learned since then) Coverity has a hard time
> when a boolean or size_t count is used to control whether or not memory
> would be freed.
>
> Looking at the logic:
>
> if (esxVI_AutoStartPowerInfo_AppendToList(>powerInfo,
> newPowerInfo) < 0) {
> goto cleanup;
> }
>
> newPowerInfo = NULL;
>
> and following it to esxVI_List_Append which on success would seemingly
> assign @newPowerInfo into the >powerInfo list, I can certainly see
> why logically setting newPowerInfo = NULL after than would seem to be
> the right thing. Of course, I see now the subtle reason why it's not a
> good idea.
>
> Restoring logic from that commit in esxDomainSetAutostart, then Coverity
> believes that @newPowerInfo is leaked from the goto cleanup at
> allocation because for some reason it has decided it must evaluate both
> true and false of a condition even though the logic only ever set the
> boolean when @newPowerInfo was placed onto the @spec->powerInfo list.
> IOW: A false positive because the human can read the code and say that
> Coverity is full of it.
>
> So either I add this to my Coverity list of false positives or in the
> "if (esxVI_AutoStartPowerInfo_Alloc() < 0 || " condition
> cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
> to cleanup, removing it from the cleanup: path, and then remove the
> "newPowerInfo = NULL;" after list insertion.
>
>
>
> John
Okay, I see what's going on. I suggest this alternative patch that
keeps the newPowerInfo = NULL line to make Coverity understand the
cleanup code, but adds a second variable to restore the original
logic. I hope this doesn't upset Coverity.
Marcos, can you give the attached patch a try? It should fix the
problems you try to fix here, by restoring the original cleanup logic.
--
Matthias Bolte
http://photron.blogspot.com
From 9d7262174142ec0cbe47ead39896a82e9155a464 Mon Sep 17 00:00:00 2001
From: Matthias Bolte
Date: Thu, 2 Aug 2018 15:57:32 +0200
Subject: [PATCH] esx: Fix double-free and freeing static strings in
esxDomainSetAutostart
Since commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5#l3393 the
newPowerInfo pointer itself is used to track the ownership of the
AutoStartPowerInfo object to make Coverity understand the code better.
This broke the code that unset some members of the AutoStartPowerInfo
object that should not be freed the normal way.
Restore the original logic by adding a second variable.
Signed-off-by: Matthias Bolte
---
src/esx/esx_driver.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index cee98ebcaf..a07a28ac93 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -3332,6 +3332,7 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
esxVI_AutoStartPowerInfo *powerInfoList = NULL;
esxVI_AutoStartPowerInfo *powerInfo = NULL;
esxVI_AutoStartPowerInfo *newPowerInfo = NULL;
+esxVI_AutoStartPowerInfo *newPowerInfo_helper = NULL;
if (esxVI_EnsureSession(priv->primary) < 0)
return -1;
@@ -3398,6 +3399,13 @@ esxDomainSetAutostart(virDomainPtr domain, int autostart)
newPowerInfo->stopDelay->value = -1; /* use system default */
newPowerInfo->stopAction = (char *)"none";
+/* Assign the new AutoStartPowerInfo to a second helper variable, so that
+ * the original variable can be used to keep track of ownership and whether
+ * or not the AutoStartPowerInfo object has to be freed explicitly in the
+ * cleanup section. The
Re: [libvirt] [PATCH v1 02/32] util: iscsi: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:17PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/viriscsi.c | 68
> +
> 1 file changed, 22 insertions(+), 46 deletions(-)
>
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 48f4106..81404f8 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -91,7 +91,7 @@ virISCSIGetSession(const char *devpath,
> VIR_AUTOFREE(char *) error = NULL;
> int exitstatus = 0;
>
> -virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode",
> +VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode",
> "session", NULL);
There are a few code misalignment issues like ^this. I'll fix them locally.
...
> @@ -576,9 +556,8 @@ virISCSINodeUpdate(const char *portal,
> const char *name,
> const char *value)
> {
> -virCommandPtr cmd = NULL;
> +VIR_AUTOPTR(virCommand) cmd = NULL;
One tiny nitpick I realized only now is that we should probably start being
consistent in what order we declare the autoclean variables, IOW I think we
should group all the autoclean variables together so visually you can
immediately tell which variables are handled automatically and which aren't. It
was all fine across this file except for ^this single occurrence, I'll fix
that.
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Availability of libvirt-4.6.0 Release Candidate 2
On 08/02/2018 03:57 AM, Peter Krempa wrote:
> On Wed, Aug 01, 2018 at 17:44:56 -0400, John Ferlan wrote:
>> On 08/01/2018 04:44 PM, Laine Stump wrote:
>
> [...]
>
>>> At any rate, there is no perfect solution in sight for the current
>>> release, so the question is whether the new (bad) behavior is better or
>>> worse than the old (also bad) behavior. My understanding is that the old
>>> behavior could lead to a config that had two devices at the same PCI
>>> address, which is definitely undesirable. The new behavior could lead to
>>> the PCI address of a newly-added device being different the next time
>>> the guest is shutdown and restarted. I would tend to prefer the latter,
>>> with the caveat that this new behavior provides a config that works
>>> (from libvirt's domain parsing POV), but might create a strange error in
>>> the guest that would be extremely difficult to troubleshoot (especially
>>> 6 months from now after we've all forgotten about this patch (and
>>> forgotten about the idea that a more complete fix was needed).
>>>
>>> So I'm undecided about my opinion. And when undecided I tend toward
>>> inaction. Now *that's* helpful, isn't it?
>>>
>>
>> Laine is stumped ;-).
>>
>> I'm still stumped over what strange error could be created. How is the
>> virtual {PCI|SCSI} address changing any different from "real hardware"
>> if someone adds something new into their physical system? Or the other
>
> Well, the issue is that you issue an API to add the device, which in
> real life would translate into plugging it into the machine.
>
> Afterwards you turn the machine off and back on. Without any API (or
> physical contact) you expect that the hardware will not move places by
> itself.
If you chose the address yourself, then it wouldn't change. IOW: If I
physically plugged into a specific spot, then no change.
But in this case, the consumer said, I don't care, choose one for me and
we did. If the consumer said LIVE and CONFIG every time, then I'd
venture to guess/assume since the algorithm is the same that the
resulting libvirt chosen address would be the same.
The problem comes when the same customer chooses CONFIG (or LIVE) at
least once before choosing CONFIG & LIVE in a followup.
>
> If the API call includes both AFFECT_LIVE and AFFECT_CONFIG we should
> make sure that the device plugged in is exactly the same. If they are
> issued separately we don't care at all though.
>
> Btw, having this analogy, specifying only AFFECT_CONFIG is probably
> similar to putting a post-it note on top of the power button for the
> next person to attach the hardware prior to powering it up.
>
Given the same physical situation of leaving a post-it note to plug this
thing in later into slot 3, but someone comes after the note is written
but before the power button is pressed and plugs something else into
slot 3 because it was just "next", then when it comes time to act upon
the post-it note where does said person plug this into since slot 3 is
taken? Do they unplug whatever was plugged into slot 3, plug the
post-it note thing into slot 3 and then plug the other thing into slot 4
or do they just plug the post-it note device into slot 4. I'd say it's
a coin flip and no worse than what we'd be doing. Conversely, if that
person plugging in live read the note and then chose slot 4, then when
it comes time to act upon the post-it note to plug at slot 3, everyone
is happy. Of course in this case, we had a human deciding to "assign"
the addresses to specific devices or in XML terms provide the
to assign the device rather than deciding upon the next available slot.
John
>
>> fun adventure, a physical move from location A to location B where
>> someone "forgot" to properly label what went where and upon reassembly
>> things are ordered differently.
>>
>> Consider some (perhaps not so) long ago SCSI devices where you'd need to
>> grab a small, sharp, pin like object to toggle switches to define the
>> address for the device when it got plugged in.
>
> Note that in that era it would be very bad in some cases when the
> hardware would change the jumper configuration by itself as sometimes
> the drivers could not autoconfigure. The addresses were put in config
> files.
>
Still trying to picture how "hardware would change the jumper
configuration by itself" without any human interaction. Those toggle
switches and connectors were a pain to deal with if you didn't have the
right tools. If only the hardware would have done it by itself, then we
wouldn't need the jumpers or toggle switches.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 01/32] util: iscsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:16PM +0530, Sukrit Bhatnagar wrote: > By making use of GNU C's cleanup attribute handled by the > VIR_AUTOFREE macro for declaring scalar variables, majority > of the VIR_FREE calls can be dropped, which in turn leads to > getting rid of most of our cleanup sections. > > Signed-off-by: Sukrit Bhatnagar > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
On 08/02/2018 05:04 AM, Matthias Bolte wrote:
> 2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza :
>> This is a new version from the last patchset sent yesterday, but now using
>> VIR_STRNDUP, instead of allocating memory manually.
>>
>> First version:
>> https://www.redhat.com/archives/libvir-list/2018-August/msg0.html
>>
>> Marcos Paulo de Souza (2):
>> esx: Do not crash SetAutoStart by double free
>> esx: Fix SetAutoStart invalid pointer free
>>
>> src/esx/esx_driver.c | 14 +++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> I see the problem, but your approach is too complicated.
>
> There is already code in place to handle those situations:
>
> 3417 cleanup:
> 3418 if (newPowerInfo) {
> 3419 newPowerInfo->key = NULL;
> 3420 newPowerInfo->startAction = NULL;
> 3421 newPowerInfo->stopAction = NULL;
> 3422 }
>
> That resets those fields to NULL to avoid double freeing and freeing
> static strings.
>
> The problem is that commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5 by
> John Frelan broke this logic, by setting newPowerInfo to NULL in the
> success path, to silence Coverity.
>
Memory recall from "Tue Sep 3 07:20:34 2013 -0400" is perhaps impossible
;-)... TL;DR, looks like the error back then was a false positive
because (as I know I've learned since then) Coverity has a hard time
when a boolean or size_t count is used to control whether or not memory
would be freed.
Looking at the logic:
if (esxVI_AutoStartPowerInfo_AppendToList(>powerInfo,
newPowerInfo) < 0) {
goto cleanup;
}
newPowerInfo = NULL;
and following it to esxVI_List_Append which on success would seemingly
assign @newPowerInfo into the >powerInfo list, I can certainly see
why logically setting newPowerInfo = NULL after than would seem to be
the right thing. Of course, I see now the subtle reason why it's not a
good idea.
Restoring logic from that commit in esxDomainSetAutostart, then Coverity
believes that @newPowerInfo is leaked from the goto cleanup at
allocation because for some reason it has decided it must evaluate both
true and false of a condition even though the logic only ever set the
boolean when @newPowerInfo was placed onto the @spec->powerInfo list.
IOW: A false positive because the human can read the code and say that
Coverity is full of it.
So either I add this to my Coverity list of false positives or in the
"if (esxVI_AutoStartPowerInfo_Alloc() < 0 || " condition
cleanup logic call esxVI_AutoStartPowerInfo_Free(); prior
to cleanup, removing it from the cleanup: path, and then remove the
"newPowerInfo = NULL;" after list insertion.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tests: mock virRandomBits to make it endian stable
virRandomBits is implemented in terms of virRandomBytes. Although we
mock virRandomBytes to give a stable value, this is not sufficient to
make virRandomBits give a stable value. The result of virRandomBits will
vary depending on endianness. Thus we mock virRandomBits to return a
stable value directly.
Signed-off-by: Daniel P. Berrangé
---
tests/virrandommock.c | 8
1 file changed, 8 insertions(+)
diff --git a/tests/virrandommock.c b/tests/virrandommock.c
index 99a55a576a..3079b8bacb 100644
--- a/tests/virrandommock.c
+++ b/tests/virrandommock.c
@@ -44,6 +44,14 @@ virRandomBytes(unsigned char *buf,
return 0;
}
+uint64_t virRandomBits(int nbits)
+{
+/* Chosen by a fair roll of a 2^64 sided dice */
+uint64_t ret = 0x0706050403020100;
+if (nbits < 64)
+ret &= ((1ULL << nbits) - 1);
+return ret;
+}
int virRandomGenerateWWN(char **wwn,
const char *virt_type ATTRIBUTE_UNUSED)
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
On Thursday, 2 August 2018 12:28:45 CEST Daniel P. Berrangé wrote: > On Thu, Aug 02, 2018 at 10:17:32AM +0200, Bjoern Walk wrote: > > Pino Toscano [2018-08-02, 10:02AM +0200]: > > > I do not think this patch is correct: we are dealing with random bytes, > > > so there is no "endianness" for them. > > > > Well, it's not incorrect either, isn't it? I agree that endianness > > doesn't matter for random data, but in the same time, it doesn't hurt to > > have the same random data generated on big- and little-endian machines. Not sure I understand -- since it's random data, you cannot have it "the same", no matter which endianness the machine has. > > And it gives an easy and future-proof fix for the mocked tests. IMHO every mocked test has its own behaviour, and thus the mocking needs to reflect that. > Lets just modijfy tests/virrandommock.c to add mocking of virRandomBits > alongside virRandomBytes. I don't see how it will help, since all virRandomBits does is calling virRandomBytes. I still did not see any complaints about my patch to fix viriscsitest (since the problem is specific for it), what about ACKing it then? -- Pino Toscano signature.asc Description: This is a digitally signed message part. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] ESX: Fixing SetAutoStart
2018-08-01 18:09 GMT+02:00 Marcos Paulo de Souza :
> This is a new version from the last patchset sent yesterday, but now using
> VIR_STRNDUP, instead of allocating memory manually.
>
> First version:
> https://www.redhat.com/archives/libvir-list/2018-August/msg0.html
>
> Marcos Paulo de Souza (2):
> esx: Do not crash SetAutoStart by double free
> esx: Fix SetAutoStart invalid pointer free
>
> src/esx/esx_driver.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
I see the problem, but your approach is too complicated.
There is already code in place to handle those situations:
3417 cleanup:
3418 if (newPowerInfo) {
3419 newPowerInfo->key = NULL;
3420 newPowerInfo->startAction = NULL;
3421 newPowerInfo->stopAction = NULL;
3422 }
That resets those fields to NULL to avoid double freeing and freeing
static strings.
The problem is that commit ae83e02f3dd7fe99fed5d8159a35b666fafeafd5 by
John Frelan broke this logic, by setting newPowerInfo to NULL in the
success path, to silence Coverity.
--
Matthias Bolte
http://photron.blogspot.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: fix test segfault when libxl configuration setup fails.
On Thu, Aug 02, 2018 at 10:35:27AM +0200, Erik Skultety wrote: > On Thu, Aug 02, 2018 at 01:06:39AM -0300, Julio Faracco wrote: > > This commit fixes a segmentation fault caused by missing conditional to > > check if libxl configuration was properly created by the test. If the > > configuration was not properly created, libxlDriverConfigNew() function > > will return NULL and cause a segfault at cfg->caps = NULL during the > > cleanup. > > > > Signed-off-by: Julio Faracco > > --- > > tests/libxlxml2domconfigtest.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c > > index 34a63e22b5..a9758c40cb 100644 > > --- a/tests/libxlxml2domconfigtest.c > > +++ b/tests/libxlxml2domconfigtest.c > > @@ -138,7 +138,8 @@ testCompareXMLToDomConfig(const char *xmlfile, > > libxl_domain_config_dispose(); > > libxl_domain_config_dispose(); > > xtl_logger_destroy(log); > > -cfg->caps = NULL; > > +if (cfg) > > +cfg->caps = NULL; > > virObjectUnref(cfg); > > return ret; > > Either this or move the "if (!(cfg - libxlDriverConfigNew()))" before the > calls > to the libxl library and return a failure right away, but I don't have a > strong > preference here, so > > Reviewed-by: Erik Skultety Oh, it's also safe for freeze, so I went ahead and pushed the patch. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: fix test segfault when libxl configuration setup fails.
On Thu, Aug 02, 2018 at 01:06:39AM -0300, Julio Faracco wrote: > This commit fixes a segmentation fault caused by missing conditional to > check if libxl configuration was properly created by the test. If the > configuration was not properly created, libxlDriverConfigNew() function > will return NULL and cause a segfault at cfg->caps = NULL during the > cleanup. > > Signed-off-by: Julio Faracco > --- > tests/libxlxml2domconfigtest.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c > index 34a63e22b5..a9758c40cb 100644 > --- a/tests/libxlxml2domconfigtest.c > +++ b/tests/libxlxml2domconfigtest.c > @@ -138,7 +138,8 @@ testCompareXMLToDomConfig(const char *xmlfile, > libxl_domain_config_dispose(); > libxl_domain_config_dispose(); > xtl_logger_destroy(log); > -cfg->caps = NULL; > +if (cfg) > +cfg->caps = NULL; > virObjectUnref(cfg); > return ret; Either this or move the "if (!(cfg - libxlDriverConfigNew()))" before the calls to the libxl library and return a failure right away, but I don't have a strong preference here, so Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] storage: add findPoolSources to iscsi_direct pool backend
From: Clementine Hayat
Change the SetContext function to be able to take the session type in
argument.
Took the function findPoolSources of iscsi backend and wired it to my
function since the formatting is essentially the same.
Signed-off-by: Clementine Hayat
---
src/storage/storage_backend_iscsi_direct.c | 179 -
1 file changed, 171 insertions(+), 8 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c
b/src/storage/storage_backend_iscsi_direct.c
index ab192730fb..fc30f2dfac 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -131,7 +131,8 @@ virStorageBackendISCSIDirectSetAuth(struct iscsi_context
*iscsi,
static int
virISCSIDirectSetContext(struct iscsi_context *iscsi,
- const char *target_name)
+ const char *target_name,
+ enum iscsi_session_type session)
{
if (iscsi_init_transport(iscsi, TCP_TRANSPORT) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -139,13 +140,15 @@ virISCSIDirectSetContext(struct iscsi_context *iscsi,
iscsi_get_error(iscsi));
return -1;
}
-if (iscsi_set_targetname(iscsi, target_name) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to set target name: %s"),
- iscsi_get_error(iscsi));
-return -1;
+if (session == ISCSI_SESSION_NORMAL) {
+if (iscsi_set_targetname(iscsi, target_name) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set target name: %s"),
+ iscsi_get_error(iscsi));
+return -1;
+}
}
-if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) < 0) {
+if (iscsi_set_session_type(iscsi, session) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set session type: %s"),
iscsi_get_error(iscsi));
@@ -400,6 +403,92 @@ virISCSIDirectDisconnect(struct iscsi_context *iscsi)
return 0;
}
+static void
+virFreeTargets(char **targets, size_t ntargets, char *target)
+{
+size_t i;
+
+VIR_FREE(target);
+for (i = 0; i < ntargets; i++)
+VIR_FREE(targets[i]);
+VIR_FREE(targets);
+}
+
+static int
+virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
+size_t *ntargets,
+char ***targets)
+{
+int ret = -1;
+struct iscsi_discovery_address *addr;
+struct iscsi_discovery_address *tmp_addr;
+size_t tmp_ntargets = 0;
+char **tmp_targets = NULL;
+
+if (!(addr = iscsi_discovery_sync(iscsi))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to discover session: %s"),
+ iscsi_get_error(iscsi));
+return ret;
+}
+*ntargets = 0;
+for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
+char *target = NULL;
+if (VIR_STRDUP(target, tmp_addr->target_name) < 0) {
+virFreeTargets(tmp_targets, tmp_ntargets, NULL);
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to allocate memory for: %s"),
+ tmp_addr->target_name);
+goto cleanup;
+}
+
+if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0) {
+virFreeTargets(tmp_targets, tmp_ntargets, target);
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to append to the list element: %s"),
+ tmp_addr->target_name);
+goto cleanup;
+}
+
+}
+
+if (tmp_ntargets) {
+*targets = tmp_targets;
+*ntargets = tmp_ntargets;
+}
+
+ret = 0;
+ cleanup:
+iscsi_free_discovery_data(iscsi, addr);
+return ret;
+}
+
+static int
+virISCSIDirectScanTargets(char *initiator_iqn,
+ char *portal,
+ size_t *ntargets,
+ char ***targets)
+{
+struct iscsi_context *iscsi = NULL;
+int ret = -1;
+
+if (!(iscsi = virISCSIDirectCreateContext(initiator_iqn)))
+goto cleanup;
+if (virISCSIDirectSetContext(iscsi, NULL, ISCSI_SESSION_DISCOVERY) < 0)
+goto cleanup;
+if (virISCSIDirectConnect(iscsi, portal) < 0)
+goto cleanup;
+if (virISCSIDirectUpdateTargets(iscsi, ntargets, targets) < 0)
+goto disconnect;
+
+ret = 0;
+ disconnect:
+virISCSIDirectDisconnect(iscsi);
+ cleanup:
+iscsi_destroy_context(iscsi);
+return ret;
+}
+
static int
virStorageBackendISCSIDirectCheckPool(virStoragePoolObjPtr pool,
bool *isActive)
@@ -408,6 +497,79 @@ virStorageBackendISCSIDirectCheckPool(virStoragePoolObjPtr
pool,
return 0;
}
+static char *
+virStorageBackendISCSIDirectFindPoolSources(const char
Re: [libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
Pino Toscano [2018-08-02, 10:02AM +0200]: > I do not think this patch is correct: we are dealing with random bytes, > so there is no "endianness" for them. Well, it's not incorrect either, isn't it? I agree that endianness doesn't matter for random data, but in the same time, it doesn't hurt to have the same random data generated on big- and little-endian machines. And it gives an easy and future-proof fix for the mocked tests. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Availability of libvirt-4.6.0 Release Candidate 2
On Wed, Aug 01, 2018 at 17:44:56 -0400, John Ferlan wrote:
> On 08/01/2018 04:44 PM, Laine Stump wrote:
[...]
> > At any rate, there is no perfect solution in sight for the current
> > release, so the question is whether the new (bad) behavior is better or
> > worse than the old (also bad) behavior. My understanding is that the old
> > behavior could lead to a config that had two devices at the same PCI
> > address, which is definitely undesirable. The new behavior could lead to
> > the PCI address of a newly-added device being different the next time
> > the guest is shutdown and restarted. I would tend to prefer the latter,
> > with the caveat that this new behavior provides a config that works
> > (from libvirt's domain parsing POV), but might create a strange error in
> > the guest that would be extremely difficult to troubleshoot (especially
> > 6 months from now after we've all forgotten about this patch (and
> > forgotten about the idea that a more complete fix was needed).
> >
> > So I'm undecided about my opinion. And when undecided I tend toward
> > inaction. Now *that's* helpful, isn't it?
> >
>
> Laine is stumped ;-).
>
> I'm still stumped over what strange error could be created. How is the
> virtual {PCI|SCSI} address changing any different from "real hardware"
> if someone adds something new into their physical system? Or the other
Well, the issue is that you issue an API to add the device, which in
real life would translate into plugging it into the machine.
Afterwards you turn the machine off and back on. Without any API (or
physical contact) you expect that the hardware will not move places by
itself.
If the API call includes both AFFECT_LIVE and AFFECT_CONFIG we should
make sure that the device plugged in is exactly the same. If they are
issued separately we don't care at all though.
Btw, having this analogy, specifying only AFFECT_CONFIG is probably
similar to putting a post-it note on top of the power button for the
next person to attach the hardware prior to powering it up.
> fun adventure, a physical move from location A to location B where
> someone "forgot" to properly label what went where and upon reassembly
> things are ordered differently.
>
> Consider some (perhaps not so) long ago SCSI devices where you'd need to
> grab a small, sharp, pin like object to toggle switches to define the
> address for the device when it got plugged in.
Note that in that era it would be very bad in some cases when the
hardware would change the jumper configuration by itself as sometimes
the drivers could not autoconfigure. The addresses were put in config
files.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
Make the generation of random bits in virRandomBits independent of the
endianness of the running architecture.
This also solves problems with the mocked random byte generation on
big-endian machines.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Bjoern Walk
---
This goes on top of Michal's fix:
https://www.redhat.com/archives/libvir-list/2018-August/msg00080.html
src/util/virrandom.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/virrandom.c b/src/util/virrandom.c
index 7915f653..26ff68f5 100644
--- a/src/util/virrandom.c
+++ b/src/util/virrandom.c
@@ -34,6 +34,7 @@
# include
#endif
+#include "virendian.h"
#include "virrandom.h"
#include "virthread.h"
#include "count-one-bits.h"
@@ -61,13 +62,16 @@ VIR_LOG_INIT("util.random");
uint64_t virRandomBits(int nbits)
{
uint64_t ret = 0;
+uint8_t val[8];
-if (virRandomBytes((unsigned char *) , sizeof(ret)) < 0) {
+if (virRandomBytes((unsigned char *) , sizeof(val)) < 0) {
/* You're already hosed, so this particular non-random value
* isn't any worse. */
return 0;
}
+ret = virReadBufInt64LE(val);
+
if (nbits < 64)
ret &= (1ULL << nbits) - 1;
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH for-3.1] s390x: remove 's390-squash-mcss' option
On Thu, 2 Aug 2018 09:33:48 +0200 Cornelia Huck wrote: > On Thu, 2 Aug 2018 09:15:24 +0200 > Cornelia Huck wrote: > > > On Thu, 2 Aug 2018 09:34:37 +0800 > > Fam Zheng wrote: > > > > > On Wed, Aug 1, 2018 at 9:18 PM Cornelia Huck wrote: > > > > > > > > > > > On Wed, 1 Aug 2018 15:11:23 +0200 > > > > Cornelia Huck wrote: > > > > > > > > > On Wed, 1 Aug 2018 14:54:51 +0200 > > > > > Cornelia Huck wrote: > > > > > > > > > > > On Wed, 1 Aug 2018 18:21:27 +0800 > > > > > > Fam Zheng wrote: > > > > > > > > > > > > > On Tue, 07/24 11:24, Cornelia Huck wrote: > > > > > > > > diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > > > > > > > > index a02d708239..1bd6c8b458 100644 > > > > > > > > --- a/hw/s390x/css-bridge.c > > > > > > > > +++ b/hw/s390x/css-bridge.c > > > > > > > > @@ -106,7 +106,6 @@ VirtualCssBus *virtual_css_bus_init(void) > > > > > > > > /* Create bus on bridge device */ > > > > > > > > bus = qbus_create(TYPE_VIRTUAL_CSS_BUS, dev, > > > > > > > > "virtual-css"); > > > > > > > > cbus = VIRTUAL_CSS_BUS(bus); > > > > > > > > > > > > > > Not used now? > > > > > > > > > > > > > > Fam > > > > > > > > > > > > Indeed, we can ditch the cbus variable. > > > > > > > > > > ...or not :) We still need it for the return value, which is processed > > > > > in ccw_init(). We could change the return code of the function to > > > > > BusState, but I'm not sure it is worth the hassle. > > > > > > > > ...but we can indeed get rid of the cbus and qbus variables in > > > > s390_ccw_realize(). > > > > > > I got this from a patchew.org test (make docker-test-clang@ubuntu): > > > > > > /tmp/qemu-test/src/hw/s390x/3270-ccw.c:103:20: error: unused variable > > > 'cbus' [-Werror,-Wunused-variable] > > > VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus); > > >^ > > > 1 error generated. > > > /tmp/qemu-test/src/rules.mak:69: recipe for target 'hw/s390x/3270-ccw.o' > > > failed > > > > Odd, why didn't I see that... > > > > ...oh wait. Why do I have -disable-werror set? I'll send a patch. > > On second thought, I'll just merge in the following: > > diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c > index cf58b81fc0..2c8d16ccf7 100644 > --- a/hw/s390x/3270-ccw.c > +++ b/hw/s390x/3270-ccw.c > @@ -98,9 +98,6 @@ static void emulated_ccw_3270_realize(DeviceState *ds, > Error **errp) > EmulatedCcw3270Class *ck = EMULATED_CCW_3270_GET_CLASS(dev); > CcwDevice *cdev = CCW_DEVICE(ds); > CCWDeviceClass *cdk = CCW_DEVICE_GET_CLASS(cdev); > -DeviceState *parent = DEVICE(cdev); > -BusState *qbus = qdev_get_parent_bus(parent); > -VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus); > SubchDev *sch; > Error *err = NULL; > > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c > index d1280bf631..cad91ee626 100644 > --- a/hw/s390x/s390-ccw.c > +++ b/hw/s390x/s390-ccw.c > @@ -67,8 +67,6 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char > *sysfsdev, Error **errp) > CcwDevice *ccw_dev = CCW_DEVICE(cdev); > CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev); > DeviceState *parent = DEVICE(ccw_dev); > -BusState *qbus = qdev_get_parent_bus(parent); > -VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus); > SubchDev *sch; > int ret; > Error *err = NULL; > ...and the one in virtio-ccw.c as well. I swear I'm not trying to break some kind of reply-to-myself record here :) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH for-3.1] s390x: remove 's390-squash-mcss' option
On Thu, 2 Aug 2018 09:15:24 +0200 Cornelia Huck wrote: > On Thu, 2 Aug 2018 09:34:37 +0800 > Fam Zheng wrote: > > > On Wed, Aug 1, 2018 at 9:18 PM Cornelia Huck wrote: > > > > > > On Wed, 1 Aug 2018 15:11:23 +0200 > > > Cornelia Huck wrote: > > > > > > > On Wed, 1 Aug 2018 14:54:51 +0200 > > > > Cornelia Huck wrote: > > > > > > > > > On Wed, 1 Aug 2018 18:21:27 +0800 > > > > > Fam Zheng wrote: > > > > > > > > > > > On Tue, 07/24 11:24, Cornelia Huck wrote: > > > > > > > diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > > > > > > > index a02d708239..1bd6c8b458 100644 > > > > > > > --- a/hw/s390x/css-bridge.c > > > > > > > +++ b/hw/s390x/css-bridge.c > > > > > > > @@ -106,7 +106,6 @@ VirtualCssBus *virtual_css_bus_init(void) > > > > > > > /* Create bus on bridge device */ > > > > > > > bus = qbus_create(TYPE_VIRTUAL_CSS_BUS, dev, "virtual-css"); > > > > > > > cbus = VIRTUAL_CSS_BUS(bus); > > > > > > > > > > > > Not used now? > > > > > > > > > > > > Fam > > > > > > > > > > Indeed, we can ditch the cbus variable. > > > > > > > > ...or not :) We still need it for the return value, which is processed > > > > in ccw_init(). We could change the return code of the function to > > > > BusState, but I'm not sure it is worth the hassle. > > > > > > ...but we can indeed get rid of the cbus and qbus variables in > > > s390_ccw_realize(). > > > > I got this from a patchew.org test (make docker-test-clang@ubuntu): > > > > /tmp/qemu-test/src/hw/s390x/3270-ccw.c:103:20: error: unused variable > > 'cbus' [-Werror,-Wunused-variable] > > VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus); > >^ > > 1 error generated. > > /tmp/qemu-test/src/rules.mak:69: recipe for target 'hw/s390x/3270-ccw.o' > > failed > > Odd, why didn't I see that... > > ...oh wait. Why do I have -disable-werror set? I'll send a patch. On second thought, I'll just merge in the following: diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c index cf58b81fc0..2c8d16ccf7 100644 --- a/hw/s390x/3270-ccw.c +++ b/hw/s390x/3270-ccw.c @@ -98,9 +98,6 @@ static void emulated_ccw_3270_realize(DeviceState *ds, Error **errp) EmulatedCcw3270Class *ck = EMULATED_CCW_3270_GET_CLASS(dev); CcwDevice *cdev = CCW_DEVICE(ds); CCWDeviceClass *cdk = CCW_DEVICE_GET_CLASS(cdev); -DeviceState *parent = DEVICE(cdev); -BusState *qbus = qdev_get_parent_bus(parent); -VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus); SubchDev *sch; Error *err = NULL; diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c index d1280bf631..cad91ee626 100644 --- a/hw/s390x/s390-ccw.c +++ b/hw/s390x/s390-ccw.c @@ -67,8 +67,6 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp) CcwDevice *ccw_dev = CCW_DEVICE(cdev); CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev); DeviceState *parent = DEVICE(ccw_dev); -BusState *qbus = qdev_get_parent_bus(parent); -VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus); SubchDev *sch; int ret; Error *err = NULL; -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: Don't overflow in virRandomBits
On 08/01/2018 04:50 PM, Eric Blake wrote: > On 08/01/2018 07:16 AM, Daniel P. Berrangé wrote: >> On Wed, Aug 01, 2018 at 01:44:32PM +0200, Michal Privoznik wrote: >>> The function is supposed to return up to 64bit long integer. In >>> order to do that it calls virRandomBytes() to fill the integer >>> with random bytes and then masks out everything but requested >>> bits. However, when doing that it shifts 1U and not 1ULL. So >>> effectively, requesting 32 random bis or more always return 0 >>> which is not random enough. >>> >>> Signed-off-by: Michal Privoznik >>> --- >>> src/util/virrandom.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/src/util/virrandom.c b/src/util/virrandom.c >>> index 01cc82a052..3c011a8615 100644 >>> --- a/src/util/virrandom.c >>> +++ b/src/util/virrandom.c >>> @@ -68,7 +68,7 @@ uint64_t virRandomBits(int nbits) >>> return 0; >>> } >>> - ret &= (1U << nbits) - 1; >>> + ret &= (1ULL << nbits) - 1; > > 1ULL << 64 is undefined in C. We need to write this as: > > if (nbits < 64) > ret &= (1ULL << nbits) - 1; > > Oops. okay. I'll post a patch for that since this is pushed already. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] viriscsi: Request more random bits for interface name
On 08/01/2018 04:48 PM, Eric Blake wrote: > On 08/01/2018 06:44 AM, Michal Privoznik wrote: >> In virStorageBackendCreateIfaceIQN() the virRandomBits() is >> called in order to use random bits to generate random name for >> new interface. However, virAsprintf() is expecting 32 bits and we >> are requesting only 30. >> >> Signed-off-by: Michal Privoznik >> --- >> src/util/viriscsi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c >> index 653b4fd932..f00aeb53a7 100644 >> --- a/src/util/viriscsi.c >> +++ b/src/util/viriscsi.c >> @@ -221,7 +221,7 @@ virStorageBackendCreateIfaceIQN(const char >> *initiatoriqn, >> if (virAsprintf(_ifacename, >> "libvirt-iface-%08llx", >> - (unsigned long long)virRandomBits(30)) < 0) >> + (unsigned long long)virRandomBits(32)) < 0) > > Are we sure that this wasn't intentional that the 2 most significant > bits are supposed to be zero (to avoid broadcast interface addresses, ro > example)? Yes we are. The random bits are used only for generating a name, not an address. The iSCSI interfaces we use here are in fact just a separate iSCSI connection, not real NICs. It's just bad naming on iSCSI side, but we are already used to that (initiator/target vs client/server), aren't we? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH for-3.1] s390x: remove 's390-squash-mcss' option
On Thu, 2 Aug 2018 09:34:37 +0800 Fam Zheng wrote: > On Wed, Aug 1, 2018 at 9:18 PM Cornelia Huck wrote: > > > > On Wed, 1 Aug 2018 15:11:23 +0200 > > Cornelia Huck wrote: > > > > > On Wed, 1 Aug 2018 14:54:51 +0200 > > > Cornelia Huck wrote: > > > > > > > On Wed, 1 Aug 2018 18:21:27 +0800 > > > > Fam Zheng wrote: > > > > > > > > > On Tue, 07/24 11:24, Cornelia Huck wrote: > > > > > > diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c > > > > > > index a02d708239..1bd6c8b458 100644 > > > > > > --- a/hw/s390x/css-bridge.c > > > > > > +++ b/hw/s390x/css-bridge.c > > > > > > @@ -106,7 +106,6 @@ VirtualCssBus *virtual_css_bus_init(void) > > > > > > /* Create bus on bridge device */ > > > > > > bus = qbus_create(TYPE_VIRTUAL_CSS_BUS, dev, "virtual-css"); > > > > > > cbus = VIRTUAL_CSS_BUS(bus); > > > > > > > > > > Not used now? > > > > > > > > > > Fam > > > > > > > > Indeed, we can ditch the cbus variable. > > > > > > ...or not :) We still need it for the return value, which is processed > > > in ccw_init(). We could change the return code of the function to > > > BusState, but I'm not sure it is worth the hassle. > > > > ...but we can indeed get rid of the cbus and qbus variables in > > s390_ccw_realize(). > > I got this from a patchew.org test (make docker-test-clang@ubuntu): > > /tmp/qemu-test/src/hw/s390x/3270-ccw.c:103:20: error: unused variable > 'cbus' [-Werror,-Wunused-variable] > VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus); >^ > 1 error generated. > /tmp/qemu-test/src/rules.mak:69: recipe for target 'hw/s390x/3270-ccw.o' > failed Odd, why didn't I see that... ...oh wait. Why do I have -disable-werror set? I'll send a patch. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH for v4.6.0 0/2] Fix some random problems
Eric Blake [2018-08-01, 09:51AM -0500]: > On 08/01/2018 07:57 AM, Bjoern Walk wrote: > > And here's the fix for the viriscsitest on big-endian machine like > > Daniel suggested. > > > From d59b254294a90c5a9ca0fb6ad29465cd0950bb61 Mon Sep 17 00:00:00 2001 > > From: Bjoern Walk > > Date: Wed, 1 Aug 2018 14:48:47 +0200 > > Subject: [PATCH] util: virrandom: make virRandomBits endian-safe > > > > Make the generation of random bits in virRandomBits independent of the > > endianness of the running architecture. > > > > This also solves problems with the mocked random byte generation on > > big-endian machines. > > > > Suggested-by: Daniel P. Berrangé > > Signed-off-by: Bjoern Walk > > --- > > src/util/virrandom.c | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > -ret &= (1ULL << nbits) - 1; > > -return ret; > > +return virReadBufInt64LE(ret) & ((1ULL << nbits) - 1); > > Needs to be rebased on top of the fix that (1ULL << 64) is not defined. Yes, agreed. But now the original patch has been pushed... -- IBM Systems Linux on Z & Virtualization Development IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
