DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C2A64624A8
Date: Wed, 19 Jul 2017 18:21:06 +0200
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Cc: David Hildenbrand <david@redhat.com>, kvm@vger.kernel.org,
        borntraeger@de.ibm.com, pbonzini@redhat.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 1/1] KVM: trigger uevents when creating or destroying
 a VM
Message-ID: <20170719162105.GB16149@potion>
References: <1499875004-17113-1-git-send-email-imbrenda@linux.vnet.ibm.com>
 <1499875004-17113-2-git-send-email-imbrenda@linux.vnet.ibm.com>
 <ccdb4556-f396-60c3-e095-fbc292223931@redhat.com>
 <20170717182756.33ed1959@p-imbrenda.boeblingen.de.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170717182756.33ed1959@p-imbrenda.boeblingen.de.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1932
Lines: 52

2017-07-17 18:27+0200, Claudio Imbrenda:
> On Mon, 17 Jul 2017 17:53:51 +0200
> David Hildenbrand <david@redhat.com> wrote:
> > > +		tmp = strchrnul(p + 1, '-');> +
> > > *tmp = '\0';
> > > +		add_uevent_var(env, "PID=%s", p);

When we're at it ... PID exists regardless of debugfs, so it would be
nice to provide it unconditionally.
(This interface looks like it was added to discourage the use of debugfs
 directory notifier in simple cases.)

I guess we cannot use task_pid_nr(current) on the KVM_EVENT_DESTROY_VM
path, so saving the PID in struct kvm would be better, IMO (it wastes
memory with larger number of VMs, but the code is harder to break).

> > > +			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.
> 
> that would be true, but then we would copy the buffer with the path
> twice, once for dentry_path_raw and once for add_uevent_var.
> 
> the env size is consistent, since that would count the bytes
> effectively present in the buffer, and note that kobject_uevent_env only
> wants a char**, no length, so there are no consistency issues. 
> 
> I agree that your solution looks better (and I had actually considered
> implementing it like that initially), but it can potentially be slower.
> I don't really have any strong preference.
> 
> Paolo: which solution do you like the most? 

I slightly prefer the simpler version as performance of the path is not
that critical.

Please note that the original patch is already in 4.13 (it was ok and my
nitpicking would just make it miss the merge window :]), so the code
improvements need to be a new patch,

thanks.