Re: [libvirt] [PATCH v11 1/5] qemu: Move TLS object remove from DetachChr to RemoveChr

2016-10-25 Thread John Ferlan 


On 10/25/2016 08:42 AM, Pavel Hrdina wrote:
> On Mon, Oct 24, 2016 at 06:46:17PM -0400, John Ferlan wrote:
>> Commit id '2c32237' added the TLS object removal to the DetachChrDevice
>> all when it should have been added to the RemoveChrDevice since that's
>> the norm for similar processing (e.g. disk)
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_hotplug.c | 44 +---
>>  1 file changed, 21 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index cf69945..31b5876 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3549,7 +3549,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>>virDomainChrDefPtr chr)
>>  {
>>  virObjectEventPtr event;
>> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>  char *charAlias = NULL;
>> +char *tlsAlias = NULL;
>>  qemuDomainObjPrivatePtr priv = vm->privateData;
>>  int ret = -1;
>>  int rc;
>> @@ -3560,7 +3562,16 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>>  if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
>>  goto cleanup;
>>  
>> +if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
>> +chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
>> +!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>> +goto cleanup;
>> +
>>  qemuDomainObjEnterMonitor(driver, vm);
>> +
>> +if (tlsAlias && qemuMonitorDelObject(priv->mon, tlsAlias) < 0)
>> +goto exit_monitor;
>> +
> 
> Those two qemu commands should be swapped as well.  This is similar to the
> issues that the next patch fixes.  The chardev uses tls object so the chardev
> should be detached before the tls object.

Took out my trusty pen and paper today and drew a large chart of attach
order, error order, and detach/remove order.

Long story short, for this one yes, moving the TLS deletion after the
chardev deletion is proper due to dependencies.

I've pushed this change.

John
> 
> ACK with that fixed.
> 
> Pavel
> 
>>  rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
>>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>  goto cleanup;
>> @@ -3579,7 +3590,13 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>>  
>>   cleanup:
>>  VIR_FREE(charAlias);
>> +VIR_FREE(tlsAlias);
>> +virObjectUnref(cfg);
>>  return ret;
>> +
>> + exit_monitor:
>> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +goto cleanup;
>>  }
>>  
>>  
>> @@ -4461,13 +4478,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr 
>> driver,
>>virDomainChrDefPtr chr)
>>  {
>>  int ret = -1;
>> -virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>  qemuDomainObjPrivatePtr priv = vm->privateData;
>>  virDomainDefPtr vmdef = vm->def;
>>  virDomainChrDefPtr tmpChr;
>> -char *objAlias = NULL;
>>  char *devstr = NULL;
>> -char *charAlias = NULL;
>>  
>>  if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
>>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> @@ -4480,26 +4494,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr 
>> driver,
>>  
>>  sa_assert(tmpChr->info.alias);
>>  
>> -if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias)))
>> -goto cleanup;
>> -
>> -if (tmpChr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
>> -tmpChr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
>> -!(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>> -goto cleanup;
>> -
>>  if (qemuBuildChrDeviceStr(, vmdef, chr, priv->qemuCaps) < 0)
>>  goto cleanup;
>>  
>>  qemuDomainMarkDeviceForRemoval(vm, >info);
>>  
>>  qemuDomainObjEnterMonitor(driver, vm);
>> -if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0)
>> -goto exit_monitor;
>> -
>> -if (objAlias && qemuMonitorDelObject(priv->mon, objAlias) < 0)
>> -goto exit_monitor;
>> -
>> +if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) {
>> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +goto cleanup;
>> +}
>>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>  goto cleanup;
>>  
>> @@ -4511,13 +4515,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>>   cleanup:
>>  qemuDomainResetDeviceRemoval(vm);
>>  VIR_FREE(devstr);
>> -VIR_FREE(charAlias);
>> -virObjectUnref(cfg);
>>  return ret;
>> -
>> - exit_monitor:
>> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> -goto cleanup;
>>  }
>>  
>>  
>> -- 
>> 2.7.4
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v11 1/5] qemu: Move TLS object remove from DetachChr to RemoveChr

2016-10-25 Thread Pavel Hrdina 
On Mon, Oct 24, 2016 at 06:46:17PM -0400, John Ferlan wrote:
> Commit id '2c32237' added the TLS object removal to the DetachChrDevice
> all when it should have been added to the RemoveChrDevice since that's
> the norm for similar processing (e.g. disk)
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_hotplug.c | 44 +---
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index cf69945..31b5876 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3549,7 +3549,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>virDomainChrDefPtr chr)
>  {
>  virObjectEventPtr event;
> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  char *charAlias = NULL;
> +char *tlsAlias = NULL;
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  int ret = -1;
>  int rc;
> @@ -3560,7 +3562,16 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>  if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
>  goto cleanup;
>  
> +if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
> +chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
> +!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
> +goto cleanup;
> +
>  qemuDomainObjEnterMonitor(driver, vm);
> +
> +if (tlsAlias && qemuMonitorDelObject(priv->mon, tlsAlias) < 0)
> +goto exit_monitor;
> +

Those two qemu commands should be swapped as well.  This is similar to the
issues that the next patch fixes.  The chardev uses tls object so the chardev
should be detached before the tls object.

ACK with that fixed.

Pavel

>  rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>  goto cleanup;
> @@ -3579,7 +3590,13 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>  
>   cleanup:
>  VIR_FREE(charAlias);
> +VIR_FREE(tlsAlias);
> +virObjectUnref(cfg);
>  return ret;
> +
> + exit_monitor:
> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +goto cleanup;
>  }
>  
>  
> @@ -4461,13 +4478,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>virDomainChrDefPtr chr)
>  {
>  int ret = -1;
> -virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  virDomainDefPtr vmdef = vm->def;
>  virDomainChrDefPtr tmpChr;
> -char *objAlias = NULL;
>  char *devstr = NULL;
> -char *charAlias = NULL;
>  
>  if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> @@ -4480,26 +4494,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>  
>  sa_assert(tmpChr->info.alias);
>  
> -if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias)))
> -goto cleanup;
> -
> -if (tmpChr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
> -tmpChr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
> -!(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
> -goto cleanup;
> -
>  if (qemuBuildChrDeviceStr(, vmdef, chr, priv->qemuCaps) < 0)
>  goto cleanup;
>  
>  qemuDomainMarkDeviceForRemoval(vm, >info);
>  
>  qemuDomainObjEnterMonitor(driver, vm);
> -if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0)
> -goto exit_monitor;
> -
> -if (objAlias && qemuMonitorDelObject(priv->mon, objAlias) < 0)
> -goto exit_monitor;
> -
> +if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) {
> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +goto cleanup;
> +}
>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>  goto cleanup;
>  
> @@ -4511,13 +4515,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>   cleanup:
>  qemuDomainResetDeviceRemoval(vm);
>  VIR_FREE(devstr);
> -VIR_FREE(charAlias);
> -virObjectUnref(cfg);
>  return ret;
> -
> - exit_monitor:
> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -goto cleanup;
>  }
>  
>  
> -- 
> 2.7.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v11 1/5] qemu: Move TLS object remove from DetachChr to RemoveChr

2016-10-24 Thread John Ferlan 
Commit id '2c32237' added the TLS object removal to the DetachChrDevice
all when it should have been added to the RemoveChrDevice since that's
the norm for similar processing (e.g. disk)

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_hotplug.c | 44 +---
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index cf69945..31b5876 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3549,7 +3549,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
   virDomainChrDefPtr chr)
 {
 virObjectEventPtr event;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 char *charAlias = NULL;
+char *tlsAlias = NULL;
 qemuDomainObjPrivatePtr priv = vm->privateData;
 int ret = -1;
 int rc;
@@ -3560,7 +3562,16 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
 if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
 goto cleanup;
 
+if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
+chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
+!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
+goto cleanup;
+
 qemuDomainObjEnterMonitor(driver, vm);
+
+if (tlsAlias && qemuMonitorDelObject(priv->mon, tlsAlias) < 0)
+goto exit_monitor;
+
 rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 goto cleanup;
@@ -3579,7 +3590,13 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
 
  cleanup:
 VIR_FREE(charAlias);
+VIR_FREE(tlsAlias);
+virObjectUnref(cfg);
 return ret;
+
+ exit_monitor:
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+goto cleanup;
 }
 
 
@@ -4461,13 +4478,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
   virDomainChrDefPtr chr)
 {
 int ret = -1;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 qemuDomainObjPrivatePtr priv = vm->privateData;
 virDomainDefPtr vmdef = vm->def;
 virDomainChrDefPtr tmpChr;
-char *objAlias = NULL;
 char *devstr = NULL;
-char *charAlias = NULL;
 
 if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -4480,26 +4494,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
 
 sa_assert(tmpChr->info.alias);
 
-if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias)))
-goto cleanup;
-
-if (tmpChr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
-tmpChr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
-!(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
-goto cleanup;
-
 if (qemuBuildChrDeviceStr(, vmdef, chr, priv->qemuCaps) < 0)
 goto cleanup;
 
 qemuDomainMarkDeviceForRemoval(vm, >info);
 
 qemuDomainObjEnterMonitor(driver, vm);
-if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0)
-goto exit_monitor;
-
-if (objAlias && qemuMonitorDelObject(priv->mon, objAlias) < 0)
-goto exit_monitor;
-
+if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) {
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+goto cleanup;
+}
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 goto cleanup;
 
@@ -4511,13 +4515,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
  cleanup:
 qemuDomainResetDeviceRemoval(vm);
 VIR_FREE(devstr);
-VIR_FREE(charAlias);
-virObjectUnref(cfg);
 return ret;
-
- exit_monitor:
-ignore_value(qemuDomainObjExitMonitor(driver, vm));
-goto cleanup;
 }
 
 
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list