From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_RED,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1C260C49361 for ; Thu, 17 Jun 2021 14:56:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F3BFC613F9 for ; Thu, 17 Jun 2021 14:56:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232021AbhFQO6e (ORCPT ); Thu, 17 Jun 2021 10:58:34 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42274 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231691AbhFQO6e (ORCPT ); Thu, 17 Jun 2021 10:58:34 -0400 Received: from mail-lj1-x234.google.com (mail-lj1-x234.google.com [IPv6:2a00:1450:4864:20::234]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DA6D7C061768 for ; Thu, 17 Jun 2021 07:56:25 -0700 (PDT) Received: by mail-lj1-x234.google.com with SMTP id s22so9441648ljg.5 for ; Thu, 17 Jun 2021 07:56:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=EbTYKOXEoTjmKWk+fpZnI99Si3buCUHA1ir4ZzP+nnI=; b=kc50UfvlreBP4U5XFPb9bvzXsic67xc5PlptbeuPCrbP14fhRwj9rOBUGLN6p+dtZX SOP1ZY5unyfg5G3JMXQ9hNEF7K9BJdNpysaExPQ4ZGnlcljp5AKYoIS2AnJWN9HA/eWW lcShCvH37ITs73OSDSewiBRwVuWFOqCDZuZLvOsSFwyv8XeY/lnBVaai2CWHMzpJ0ZWY Y53GpWGGPDQii1rEcnb3vD8rPnS7JYgrBF/1Tz+6xjdeaKCB2GHwPT9jPUZBQxOpU2vJ uVx93iAJ9KIPSlbLvTT+iKfClmYBRlq2GejTdrownxtxHdZu6Y8+qgMycGlLKMFEmk5n Fe2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=EbTYKOXEoTjmKWk+fpZnI99Si3buCUHA1ir4ZzP+nnI=; b=X6Yae5MNGJoKHNmzC3qnn9f1JNVYXZ9Xh1xPS0rL+diXNyfZi/mi7+Dj2/FneA4w5H Y26P8F8lGlSCqJv+Oc3TobY9VFRAjAOqia3+jczPwFGsUpQXq9Unes/ODUPw2gV8GSYR Th9NCNohcAyQCvZtkGK5dhBQyfU6LPq2EeVs7DPiPlWravMxLcYpTopxQWi6II3bI3x4 Ve4JfWtnM+id+KDKSWcdZj5zGol+NcsekuivIggc2pE1MhhAZfPoir/lyqWHUUXib8UP DQnkUkWeCDGswiRrsfnxikYWtFH1VVKIguagj2FFe8NpJLzFi33y/cgdmqvjzxYefgfv VOog== X-Gm-Message-State: AOAM530Sh4kntwopHtWGO6Rk2oRuxgpJGfE6rsimCal/H3IZ8iUhfVMM 4tEC3YFvyh7mV+fURRLzTndD+bWnsJr/A/z9meWf1g== X-Google-Smtp-Source: ABdhPJyHZSoRzwk/uvauvQQuu1bVwpP27gPIjPYDoJYNtfXQcbgPRMFOMqegaGb6vFsOuSX995igEG4xKcInEvg9Wdg= X-Received: by 2002:a2e:9b8f:: with SMTP id z15mr4952786lji.304.1623941783847; Thu, 17 Jun 2021 07:56:23 -0700 (PDT) MIME-Version: 1.0 References: <20210617044146.2667540-1-jingzhangos@google.com> <20210617044146.2667540-3-jingzhangos@google.com> In-Reply-To: From: Jing Zhang Date: Thu, 17 Jun 2021 09:56:12 -0500 Message-ID: Subject: Re: [PATCH v10 2/5] KVM: stats: Add fd-based API to read binary stats data To: Greg KH Cc: KVM , KVMARM , LinuxMIPS , KVMPPC , LinuxS390 , Linuxkselftest , Paolo Bonzini , Marc Zyngier , James Morse , Julien Thierry , Suzuki K Poulose , Will Deacon , Huacai Chen , Aleksandar Markovic , Thomas Bogendoerfer , Paul Mackerras , Christian Borntraeger , Janosch Frank , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Peter Shier , Oliver Upton , David Rientjes , Emanuele Giuseppe Esposito , David Matlack , Ricardo Koller , Krish Sadhukhan , Fuad Tabba Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-mips@vger.kernel.org Hi Greg, On Thu, Jun 17, 2021 at 2:24 AM Greg KH wrote: > > On Thu, Jun 17, 2021 at 04:41:43AM +0000, Jing Zhang wrote: > > Provides a file descriptor per VM to read VM stats info/data. > > Provides a file descriptor per vCPU to read vCPU stats info/data. > > Shouldn't this be two separate patches, one for each thing as these are > two different features being added? > Actually, it is really not easy to separate this change into two patches even by following Paolo's suggestion. And it would be a surprise to userspace to see only VM stats, no VCPU stats. I guess it is the text that caused the confusion, I'll change the commit message for this. > Anyway, an implementation question for both of these: > > > +static ssize_t kvm_stats_read(struct _kvm_stats_header *header, > > + struct _kvm_stats_desc *desc, void *stats, size_t size_stats, > > + char __user *user_buffer, size_t size, loff_t *offset) > > +{ > > + ssize_t copylen, len, remain = size; > > You are "burying" the fact that remain is initialized here, not nice, I > totally missed it when reading this the first time. > > This should be: > ssize_t copylen; > ssize_t len; > ssize_t remain = size; > to be obvious. > > Remember you will be looking at this code for the next 20 years, make it > easy to read. > Nice! Will do. > > + size_t size_header, size_desc; > > + loff_t pos = *offset; > > + char __user *dest = user_buffer; > > + void *src; > > + > > + size_header = sizeof(*header); > > + size_desc = header->header.count * sizeof(*desc); > > + > > + len = size_header + size_desc + size_stats - pos; > > + len = min(len, remain); > > + if (len <= 0) > > + return 0; > > + remain = len; > > + > > + /* Copy kvm stats header */ > > + copylen = size_header - pos; > > + copylen = min(copylen, remain); > > + if (copylen > 0) { > > + src = (void *)header + pos; > > + if (copy_to_user(dest, src, copylen)) > > + return -EFAULT; > > + remain -= copylen; > > + pos += copylen; > > + dest += copylen; > > + } > > I thought you said that you would not provide the header for each read, > if you keep reading from the fd. It looks like you are adding it here > to each read, or is there some "magic" with pos happening here that I do > not understand? > > And if there is "magic" with pos, you should document it as it's not > very obvious :) > Will do. > > + /* Copy kvm stats descriptors */ > > + copylen = header->header.desc_offset + size_desc - pos; > > + copylen = min(copylen, remain); > > + if (copylen > 0) { > > + src = (void *)desc + pos - header->header.desc_offset; > > + if (copy_to_user(dest, src, copylen)) > > + return -EFAULT; > > + remain -= copylen; > > + pos += copylen; > > + dest += copylen; > > + } > > + /* Copy kvm stats values */ > > New lines between code blocks of doing things? > Will add lines between code blocks. > And again, why copy the decriptor again? or is it being skipped > somehow? Ah, I think I see how it's being skipped, if I look really > closely. But again, it's not obvious, and I could be wrong. Please > document this REALLY well. > > Write code for the developer first, compiler second. Again, you are > going to be maintaining it for 20+ years, think of your future self... > > Sure, will document it. > > + copylen = header->header.data_offset + size_stats - pos; > > + copylen = min(copylen, remain); > > + if (copylen > 0) { > > + src = stats + pos - header->header.data_offset; > > + if (copy_to_user(dest, src, copylen)) > > + return -EFAULT; > > + remain -= copylen; > > + pos += copylen; > > + dest += copylen; > > + } > > + > > + *offset = pos; > > + return len; > > +} > > thanks, > > greg k-h Thanks, Jing From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_RED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 255C0C2B9F4 for ; Thu, 17 Jun 2021 14:56:31 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 9881A613E9 for ; Thu, 17 Jun 2021 14:56:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9881A613E9 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id E49774B090; Thu, 17 Jun 2021 10:56:29 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id wva1nG33Q+7j; Thu, 17 Jun 2021 10:56:28 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id BA1A849E5F; Thu, 17 Jun 2021 10:56:28 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id C813940821 for ; Thu, 17 Jun 2021 10:56:26 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id aMRUAqRRPGNf for ; Thu, 17 Jun 2021 10:56:25 -0400 (EDT) Received: from mail-lj1-f173.google.com (mail-lj1-f173.google.com [209.85.208.173]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 89A2D40233 for ; Thu, 17 Jun 2021 10:56:25 -0400 (EDT) Received: by mail-lj1-f173.google.com with SMTP id c11so9432588ljd.6 for ; Thu, 17 Jun 2021 07:56:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=EbTYKOXEoTjmKWk+fpZnI99Si3buCUHA1ir4ZzP+nnI=; b=kc50UfvlreBP4U5XFPb9bvzXsic67xc5PlptbeuPCrbP14fhRwj9rOBUGLN6p+dtZX SOP1ZY5unyfg5G3JMXQ9hNEF7K9BJdNpysaExPQ4ZGnlcljp5AKYoIS2AnJWN9HA/eWW lcShCvH37ITs73OSDSewiBRwVuWFOqCDZuZLvOsSFwyv8XeY/lnBVaai2CWHMzpJ0ZWY Y53GpWGGPDQii1rEcnb3vD8rPnS7JYgrBF/1Tz+6xjdeaKCB2GHwPT9jPUZBQxOpU2vJ uVx93iAJ9KIPSlbLvTT+iKfClmYBRlq2GejTdrownxtxHdZu6Y8+qgMycGlLKMFEmk5n Fe2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=EbTYKOXEoTjmKWk+fpZnI99Si3buCUHA1ir4ZzP+nnI=; b=hxF+PDHqioM2/srUXf1scxx5f3LRmJtUtfGCIJ5ihXaNT7Lfwx0Ej8p2oG/0pOvqm0 0uLxVMDomoO666NsJlTnfE3t1X6onljjXyj3Id17Hgu6Bt5r9yR+A/BW+t8rweqhROPE fdEkus/qsNk+1Mtqf4p/CN9xb9JEtZKgRjNBtQl8144Zohz28KP/BwLjoqx2IX/kb2fT IKYfy4MOUHUtp0TdqyaRK4EEdiingKZgQ0plDR4B/V2AnjzpRs/MaS+BaZe+IPB2MAKv UalOmi7tRpMNX/8IekhTqEJ3FTOvxghmQkw0luh8Q4PBzuBzG8pORh+bP4KvCtQrg1JP dJqQ== X-Gm-Message-State: AOAM533WpSNgsMI1dRydpxEFSpzxMXhWCMcnSTuucF0R5ic3q7IML09q mO0MRnBgYPIOGDbW60/YLFZ1lriYAM17zb432+5whA== X-Google-Smtp-Source: ABdhPJyHZSoRzwk/uvauvQQuu1bVwpP27gPIjPYDoJYNtfXQcbgPRMFOMqegaGb6vFsOuSX995igEG4xKcInEvg9Wdg= X-Received: by 2002:a2e:9b8f:: with SMTP id z15mr4952786lji.304.1623941783847; Thu, 17 Jun 2021 07:56:23 -0700 (PDT) MIME-Version: 1.0 References: <20210617044146.2667540-1-jingzhangos@google.com> <20210617044146.2667540-3-jingzhangos@google.com> In-Reply-To: From: Jing Zhang Date: Thu, 17 Jun 2021 09:56:12 -0500 Message-ID: Subject: Re: [PATCH v10 2/5] KVM: stats: Add fd-based API to read binary stats data To: Greg KH Cc: KVM , David Hildenbrand , Paul Mackerras , Linuxkselftest , Claudio Imbrenda , Will Deacon , KVMARM , Emanuele Giuseppe Esposito , LinuxS390 , Janosch Frank , Marc Zyngier , Huacai Chen , Christian Borntraeger , Aleksandar Markovic , David Rientjes , KVMPPC , Krish Sadhukhan , David Matlack , Jim Mattson , Thomas Bogendoerfer , Sean Christopherson , Cornelia Huck , Peter Shier , LinuxMIPS , Paolo Bonzini , Vitaly Kuznetsov X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu Hi Greg, On Thu, Jun 17, 2021 at 2:24 AM Greg KH wrote: > > On Thu, Jun 17, 2021 at 04:41:43AM +0000, Jing Zhang wrote: > > Provides a file descriptor per VM to read VM stats info/data. > > Provides a file descriptor per vCPU to read vCPU stats info/data. > > Shouldn't this be two separate patches, one for each thing as these are > two different features being added? > Actually, it is really not easy to separate this change into two patches even by following Paolo's suggestion. And it would be a surprise to userspace to see only VM stats, no VCPU stats. I guess it is the text that caused the confusion, I'll change the commit message for this. > Anyway, an implementation question for both of these: > > > +static ssize_t kvm_stats_read(struct _kvm_stats_header *header, > > + struct _kvm_stats_desc *desc, void *stats, size_t size_stats, > > + char __user *user_buffer, size_t size, loff_t *offset) > > +{ > > + ssize_t copylen, len, remain = size; > > You are "burying" the fact that remain is initialized here, not nice, I > totally missed it when reading this the first time. > > This should be: > ssize_t copylen; > ssize_t len; > ssize_t remain = size; > to be obvious. > > Remember you will be looking at this code for the next 20 years, make it > easy to read. > Nice! Will do. > > + size_t size_header, size_desc; > > + loff_t pos = *offset; > > + char __user *dest = user_buffer; > > + void *src; > > + > > + size_header = sizeof(*header); > > + size_desc = header->header.count * sizeof(*desc); > > + > > + len = size_header + size_desc + size_stats - pos; > > + len = min(len, remain); > > + if (len <= 0) > > + return 0; > > + remain = len; > > + > > + /* Copy kvm stats header */ > > + copylen = size_header - pos; > > + copylen = min(copylen, remain); > > + if (copylen > 0) { > > + src = (void *)header + pos; > > + if (copy_to_user(dest, src, copylen)) > > + return -EFAULT; > > + remain -= copylen; > > + pos += copylen; > > + dest += copylen; > > + } > > I thought you said that you would not provide the header for each read, > if you keep reading from the fd. It looks like you are adding it here > to each read, or is there some "magic" with pos happening here that I do > not understand? > > And if there is "magic" with pos, you should document it as it's not > very obvious :) > Will do. > > + /* Copy kvm stats descriptors */ > > + copylen = header->header.desc_offset + size_desc - pos; > > + copylen = min(copylen, remain); > > + if (copylen > 0) { > > + src = (void *)desc + pos - header->header.desc_offset; > > + if (copy_to_user(dest, src, copylen)) > > + return -EFAULT; > > + remain -= copylen; > > + pos += copylen; > > + dest += copylen; > > + } > > + /* Copy kvm stats values */ > > New lines between code blocks of doing things? > Will add lines between code blocks. > And again, why copy the decriptor again? or is it being skipped > somehow? Ah, I think I see how it's being skipped, if I look really > closely. But again, it's not obvious, and I could be wrong. Please > document this REALLY well. > > Write code for the developer first, compiler second. Again, you are > going to be maintaining it for 20+ years, think of your future self... > > Sure, will document it. > > + copylen = header->header.data_offset + size_stats - pos; > > + copylen = min(copylen, remain); > > + if (copylen > 0) { > > + src = stats + pos - header->header.data_offset; > > + if (copy_to_user(dest, src, copylen)) > > + return -EFAULT; > > + remain -= copylen; > > + pos += copylen; > > + dest += copylen; > > + } > > + > > + *offset = pos; > > + return len; > > +} > > thanks, > > greg k-h Thanks, Jing _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jing Zhang Date: Thu, 17 Jun 2021 14:56:12 +0000 Subject: Re: [PATCH v10 2/5] KVM: stats: Add fd-based API to read binary stats data Message-Id: List-Id: References: <20210617044146.2667540-1-jingzhangos@google.com> <20210617044146.2667540-3-jingzhangos@google.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Greg KH Cc: KVM , KVMARM , LinuxMIPS , KVMPPC , LinuxS390 , Linuxkselftest , Paolo Bonzini , Marc Zyngier , James Morse , Julien Thierry , Suzuki K Poulose , Will Deacon , Huacai Chen , Aleksandar Markovic , Thomas Bogendoerfer , Paul Mackerras , Christian Borntraeger , Janosch Frank , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Peter Shier , Oliver Upton , David Rientjes , Emanuele Giuseppe Esposito , David Matlack , Ricardo Koller , Krish Sadhukhan , Fuad Tabba Hi Greg, On Thu, Jun 17, 2021 at 2:24 AM Greg KH wrote: > > On Thu, Jun 17, 2021 at 04:41:43AM +0000, Jing Zhang wrote: > > Provides a file descriptor per VM to read VM stats info/data. > > Provides a file descriptor per vCPU to read vCPU stats info/data. > > Shouldn't this be two separate patches, one for each thing as these are > two different features being added? > Actually, it is really not easy to separate this change into two patches even by following Paolo's suggestion. And it would be a surprise to userspace to see only VM stats, no VCPU stats. I guess it is the text that caused the confusion, I'll change the commit message for this. > Anyway, an implementation question for both of these: > > > +static ssize_t kvm_stats_read(struct _kvm_stats_header *header, > > + struct _kvm_stats_desc *desc, void *stats, size_t size_stats, > > + char __user *user_buffer, size_t size, loff_t *offset) > > +{ > > + ssize_t copylen, len, remain = size; > > You are "burying" the fact that remain is initialized here, not nice, I > totally missed it when reading this the first time. > > This should be: > ssize_t copylen; > ssize_t len; > ssize_t remain = size; > to be obvious. > > Remember you will be looking at this code for the next 20 years, make it > easy to read. > Nice! Will do. > > + size_t size_header, size_desc; > > + loff_t pos = *offset; > > + char __user *dest = user_buffer; > > + void *src; > > + > > + size_header = sizeof(*header); > > + size_desc = header->header.count * sizeof(*desc); > > + > > + len = size_header + size_desc + size_stats - pos; > > + len = min(len, remain); > > + if (len <= 0) > > + return 0; > > + remain = len; > > + > > + /* Copy kvm stats header */ > > + copylen = size_header - pos; > > + copylen = min(copylen, remain); > > + if (copylen > 0) { > > + src = (void *)header + pos; > > + if (copy_to_user(dest, src, copylen)) > > + return -EFAULT; > > + remain -= copylen; > > + pos += copylen; > > + dest += copylen; > > + } > > I thought you said that you would not provide the header for each read, > if you keep reading from the fd. It looks like you are adding it here > to each read, or is there some "magic" with pos happening here that I do > not understand? > > And if there is "magic" with pos, you should document it as it's not > very obvious :) > Will do. > > + /* Copy kvm stats descriptors */ > > + copylen = header->header.desc_offset + size_desc - pos; > > + copylen = min(copylen, remain); > > + if (copylen > 0) { > > + src = (void *)desc + pos - header->header.desc_offset; > > + if (copy_to_user(dest, src, copylen)) > > + return -EFAULT; > > + remain -= copylen; > > + pos += copylen; > > + dest += copylen; > > + } > > + /* Copy kvm stats values */ > > New lines between code blocks of doing things? > Will add lines between code blocks. > And again, why copy the decriptor again? or is it being skipped > somehow? Ah, I think I see how it's being skipped, if I look really > closely. But again, it's not obvious, and I could be wrong. Please > document this REALLY well. > > Write code for the developer first, compiler second. Again, you are > going to be maintaining it for 20+ years, think of your future self... > > Sure, will document it. > > + copylen = header->header.data_offset + size_stats - pos; > > + copylen = min(copylen, remain); > > + if (copylen > 0) { > > + src = stats + pos - header->header.data_offset; > > + if (copy_to_user(dest, src, copylen)) > > + return -EFAULT; > > + remain -= copylen; > > + pos += copylen; > > + dest += copylen; > > + } > > + > > + *offset = pos; > > + return len; > > +} > > thanks, > > greg k-h Thanks, Jing