Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751484AbdGQQ2K (ORCPT ); Mon, 17 Jul 2017 12:28:10 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:34545 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751359AbdGQQ2G (ORCPT ); Mon, 17 Jul 2017 12:28:06 -0400 Date: Mon, 17 Jul 2017 18:27:56 +0200 From: Claudio Imbrenda To: David Hildenbrand Cc: 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 In-Reply-To: References: <1499875004-17113-1-git-send-email-imbrenda@linux.vnet.ibm.com> <1499875004-17113-2-git-send-email-imbrenda@linux.vnet.ibm.com> Organization: IBM X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17071716-0016-0000-0000-000004D7B5EF X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17071716-0017-0000-0000-0000280C6686 Message-Id: <20170717182756.33ed1959@p-imbrenda.boeblingen.de.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-07-17_12:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000 definitions=main-1707170263 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2160 Lines: 73 On Mon, 17 Jul 2017 17:53:51 +0200 David Hildenbrand wrote: > > + 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. yeah it would probably look cleaner, I'll fix it. > > + > > + 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. I'll fix that too > > + 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. 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? [...]