Date: Mon, 10 Jul 2017 13:53:11 +0200
From: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
To: David Hildenbrand <david@redhat.com>
Cc: kvm@vger.kernel.org, borntraeger@de.ibm.com, pbonzini@redhat.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/1] KVM: trigger uevents when starting or stopping a
 VM
In-Reply-To: <852e01e7-81f4-7fcb-dc55-80f4aed029c0@redhat.com>
References: <1499678444-21879-1-git-send-email-imbrenda@linux.vnet.ibm.com>
        <1499678444-21879-2-git-send-email-imbrenda@linux.vnet.ibm.com>
        <852e01e7-81f4-7fcb-dc55-80f4aed029c0@redhat.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Message-Id: <20170710135311.24600ce2@p-imbrenda.boeblingen.de.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1894
Lines: 64

On Mon, 10 Jul 2017 11:40:55 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 10.07.2017 11:20, Claudio Imbrenda wrote:
> 
> Minor minor nit:
> 
> The subject should state "creating or destroying a VM"

I'll fix it

[...]

> > +static void kvm_uevent_notify_change(unsigned int type, struct kvm
> > *kvm) +{
> > +	char cbuf[32], abuf[32], pidbuf[32], evbuf[16];  
> 
> do we really need that much space for a pid?

unfortunately yes. we don't have access to the pid when destroying the
VM, so I take it from the debugfs entry, and that has the format
"%d-%d", with pid and file descriptor, both can be 10 bytes long.

> > +	const char pathvar[11] = "STATS_PATH=";
> > +	char *ptr[6] = {cbuf, abuf, pidbuf, NULL, NULL, NULL};
> > +	char *tmp, *pathbuf;
> > +	unsigned long long created, active;
> > +	int idx = 3;
> > +
> > +	if (!kvm_dev.this_device || !kvm || !kvm->debugfs_dentry)
> > +		return;
> > +
> > +	spin_lock(&kvm_lock);
> > +	if (type == KVM_EVENT_CREATE_VM) {
> > +		kvm_createvm_count++;
> > +		kvm_active_vms++;
> > +	} else if (type == KVM_EVENT_DESTROY_VM) {
> > +		kvm_active_vms--;
> > +	}
> > +	created = kvm_createvm_count;
> > +	active = kvm_active_vms;
> > +	spin_unlock(&kvm_lock);
> > +
> > +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> > +	if (pathbuf) {
> > +		tmp = dentry_path_raw(kvm->debugfs_dentry,
> > +				      pathbuf + sizeof(pathvar),
> > +				      PATH_MAX - sizeof(pathvar));
> > +		if (!IS_ERR(tmp)) {
> > +			memcpy(tmp - sizeof(pathvar), pathvar,
> > sizeof(pathvar));
> > +			ptr[idx++] = tmp - sizeof(pathvar);
> > +		}
> > +	}
> > +	snprintf(cbuf, sizeof(cbuf), "CREATED=%llu", created);  
> 
> Did you think about using struct kobj_uevent_env / add_uevent_var()?

for what I could see, that only works with a more invasive patch. I'll
see what I can do.
 
> But most probably the problem here is special handling for
> dentry_path_raw().