On 01/13/2017 12:26 PM, Andrea Bolognani wrote:
> On Fri, 2017-01-13 at 11:12 +0100, Michal Privoznik wrote:
> [...]
>> @@ -7023,10 +7029,34 @@ qemuDomainCreateDevice(const char *device,
>> goto cleanup;
>> }
>>
>> +#ifdef WITH_SELINUX
>> + if (getfilecon_raw(canonDevicePath, &tcon) < 0 &&
>> + (errno != ENOTSUP && errno != ENODATA)) {
>> + virReportSystemError(errno,
>> + _("Unable to get SELinux label on %s"), canonDevicePath);
>
> s/get SELinux label on/get SELinux label from/
>
> One more occurrence in the patch.
>
>> + goto cleanup;
>> + }
>> +
>> + if (tcon &&
>> + setfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *) tcon) < 0) {
>> + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
>> + if (errno != EOPNOTSUPP && errno != ENOTSUP) {
>> + VIR_WARNINGS_RESET
>> + virReportSystemError(errno,
>> + _("Unable to set SELinux label on %s"),
>> + devicePath);
>
> Please decide whether you want the argument to %s on the same
> line as the format string or on the next, and stick with it :)
>
> [...]
>> @@ -7571,6 +7617,9 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
>> cleanup:
>> if (ret < 0 && delDevice)
>> unlink(data->file);
>> +#ifdef WITH_SELINUX
>> + freecon(data->tcon);
>> +#endif
>
> I don't think you should free the SELinux context...
>
>> virFileFreeACLs(&data->acl);
>
> ... or the ACLs, for that matter, on failure: the caller
> will free them already if the helper fails, which is good
> because whoever allocates the memory should be responsible
> for releasing it.
In fact I need to. This function is ran in a separate process. Therefore
there is no double free. On the other hand, with return from this
function the process ends anyway, so if we don't free it kernel will.
I'm keeping it for the time being.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
