Re: [libvirt] [PATCH 1/2] qemu: fix some small issue in qemuProcessAttach
On 12/01/2014 06:27 PM, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 11:17:54AM +0100, Martin Kletzander wrote:
> On Mon, Dec 01, 2014 at 05:54:35PM +0800, Luyao Huang wrote:
>> There are some small issue in qemuProcessAttach:
>>
>> 1.Fix virSecurityManagerGetProcessLabel always get pid = 0,
>> move 'vm->pid = pid' before call virSecurityManagerGetProcessLabel.
>>
>> 2.Use virSecurityManagerGenLabel to get image label.
>>
>> 3.Fix always set selinux label for other security driver label.
>>
>> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
>> ---
>> src/qemu/qemu_process.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>
> It looks like we were doing everything already, but we just did it
> wrong. Nice catch! ACK.
>
Oh, I spoke too soon. Two minor things are wrong with this patch that
I'll fixup before pushing:
1) The commit message summary is not very descriptive. When someone
will go over the git log he/she won't figure out what was the deal
from "fix some small issue:.
Thanks for pointing out, BTW, maybe qemuprocessattach will failed after
this patch
without the patch 2/2, because we should give a way to get process uid
and gid in
virSecurityDACGetProcessLabel otherwise it will always return -1 without
a error
settings.
I will edit Patch 2 and give a v2 in these days (otherwise i cannot use
qemu-attach : )).
2) see below...
> Martin
>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 382d802..ee95adb 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -5255,6 +5255,8 @@ int qemuProcessAttach(virConnectPtr conn
>> ATTRIBUTE_UNUSED,
>> if (VIR_STRDUP(priv->pidfile, pidfile) < 0)
>> goto error;
>>
>> + vm->pid = pid;
>> +
>> VIR_DEBUG("Detect security driver config");
>> sec_managers =
>> virSecurityManagerGetNested(driver->securityManager);
>> if (sec_managers == NULL)
>> @@ -5272,7 +5274,7 @@ int qemuProcessAttach(virConnectPtr conn
>> ATTRIBUTE_UNUSED,
>> seclabeldef->type = VIR_DOMAIN_SECLABEL_STATIC;
>> if (VIR_ALLOC(seclabel) < 0)
>> goto error;
>> - if (virSecurityManagerGetProcessLabel(driver->securityManager,
>> + if (virSecurityManagerGetProcessLabel(sec_managers[i],
>> vm->def, vm->pid,
>> seclabel) < 0)
>> goto error;
>>
>> @@ -5290,6 +5292,10 @@ int qemuProcessAttach(virConnectPtr conn
>> ATTRIBUTE_UNUSED,
>> }
>> }
>>
>> + if (virSecurityManagerGenLabel(driver->securityManager,
>> vm->def) < 0) {
>> + goto error;
>> + }
>> +
This won't pass our syntax-check.
Oh that is a accident, i removed the error
settings but forgot the {}
when i sent this patch : )
>> VIR_DEBUG("Creating domain log file");
>> if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0)
>> goto error;
>> @@ -5334,8 +5340,6 @@ int qemuProcessAttach(virConnectPtr conn
>> ATTRIBUTE_UNUSED,
>>
>> qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH,
>> logfile);
>>
>> - vm->pid = pid;
>> -
>> VIR_DEBUG("Waiting for monitor to show up");
>> if (qemuProcessWaitForMonitor(driver, vm, QEMU_ASYNC_JOB_NONE,
>> priv->qemuCaps, -1) < 0)
>> goto error;
>> --
>> 1.8.3.1
>>
>> --
>> libvir-list mailing list
>> libvir-list(a)redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list