DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D11AD7CB8B
Subject: Re: [PATCH v5 1/1] KVM: trigger uevents when creating or destroying a
 VM
To: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>, kvm@vger.kernel.org
Cc: borntraeger@de.ibm.com, pbonzini@redhat.com,
        linux-kernel@vger.kernel.org
References: <1499875004-17113-1-git-send-email-imbrenda@linux.vnet.ibm.com>
 <1499875004-17113-2-git-send-email-imbrenda@linux.vnet.ibm.com>
From: David Hildenbrand <david@redhat.com>
Organization: Red Hat GmbH
Message-ID: <ccdb4556-f396-60c3-e095-fbc292223931@redhat.com>
Date: Mon, 17 Jul 2017 17:53:51 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.0
MIME-Version: 1.0
In-Reply-To: <1499875004-17113-2-git-send-email-imbrenda@linux.vnet.ibm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1798
Lines: 77


> +	add_uevent_var(env, "CREATED=%llu", created);
> +	add_uevent_var(env, "COUNT=%llu", active);

I like that much better.

> +
> +	if (type == KVM_EVENT_CREATE_VM)
> +		add_uevent_var(env, "EVENT=create");
> +	else if (type == KVM_EVENT_DESTROY_VM)
> +		add_uevent_var(env, "EVENT=destroy");
> +
> +	if (kvm->debugfs_dentry) {
> +		char p[ITOA_MAX_LEN];

I'd move this also to the top of the function. Or move tmp and pathbuf
in here, so you could also move them to this place.

> +
> +		snprintf(p, sizeof(p), "%s", kvm->debugfs_dentry->d_name.name);

Probably just me, but I prefer ARRAY_SIZE() instead of sizeof() for any
kind of array sizes.

> +		tmp = strchrnul(p + 1, '-');> +		*tmp = '\0';
> +		add_uevent_var(env, "PID=%s", p);
> +		pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> +		if (pathbuf) {
> +			/* sizeof counts the final '\0' */
> +			int len = sizeof("STATS_PATH=") - 1;
> +			const char *pvar = "STATS_PATH=";

Can't you avoid that? See next paragraph.


> +
> +			tmp = dentry_path_raw(kvm->debugfs_dentry,
> +					      pathbuf + len,
> +					      PATH_MAX - len);
> +			if (!IS_ERR(tmp)) {

Why not

tmp = dentry_path_raw(kvm->debugfs_dentry, pathbuf, PATH_MAX);
if (!IS_ERR(tmp)) {
	add_uevent_var(env, "STATS_PATH=s", tmp);
}

and avoid len / pvar / memcpy? And keep internal env size checking
consistent.

(are my tired eyes missing something? :) )


> +				memcpy(tmp - len, pvar, len);
> +				env->envp[env->envp_idx++] = tmp - len;
> +			}
> +		}
> +	}
> +	/* no need for checks, since we are adding at most only 5 keys */
> +	env->envp[env->envp_idx++] = NULL;
> +	kobject_uevent_env(&kvm_dev.this_device->kobj, KOBJ_CHANGE, env->envp);
> +	kfree(env);
> +	kfree(pathbuf);
> +}
> +
>  static int kvm_init_debug(void)
>  {
>  	int r = -EEXIST;
> 


-- 

Thanks,

David