[libvirt] [PATCH 1/2] qemu: fix some small issue in qemuProcessAttach

[Luyao Huang]

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 at 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 at redhat.com
>>> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
>
>> -- 
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list