Re: [PATCH v1 3/4] Add write_kdump_page

From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
To: zhouwj-fnst@cn.fujitsu.com
Cc: kexec@lists.infradead.org
Subject: Re: [PATCH v1 3/4] Add write_kdump_page
Date: Wed, 15 Jul 2015 12:09:57 +0900 (JST)	[thread overview]
Message-ID: <20150715.120957.106666381.d.hatayama@jp.fujitsu.com> (raw)
In-Reply-To: <1436495333-22892-4-git-send-email-zhouwj-fnst@cn.fujitsu.com>

From: Zhou Wenjian <zhouwj-fnst@cn.fujitsu.com>
Subject: [PATCH v1 3/4] Add write_kdump_page
Date: Fri, 10 Jul 2015 10:28:52 +0800

> write_kdump_page is used to write page when generating kdump core.
> It mainly optimizes the way of generating incomplete core.
> 
> Signed-off-by: Zhou Wenjian <zhouwj-fnst@cn.fujitsu.com>
> ---
>  makedumpfile.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index e8b52f4..74bf83e 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -6324,6 +6324,59 @@ get_pfn_offset(void *buf, struct cache_data *cd_page){
>  }
>  
>  int
> +write_kdump_page(struct cache_data *cd_header, struct cache_data *cd_page,
> +		struct page_desc *pd, void *page_data)
> +{
> +	int written_pfns_size;
> +
> +	/*
> +	 * if either cd_header or cd_page is nearly full,
> +	 * write the buffer cd_header into dumpfile and then write the cd_page.
> +	 */

Please begin comments by capital letter. They should be formal.

Also, please explain why. What currently this comment says is obvious
from the below.

> +	if ( cd_header->buf_size + sizeof(pd) > cd_header->cache_size

No space needed here.

Also, sizeof(pd) should have been sizeof(*pd), right?

if (cd_header->buf_size + sizeof(*pd) > cd_header->cache_size
 		|| cd_page->buf_size + pd->size > cd_page->cache_size){

> +		|| cd_page->buf_size + pd->size > cd_page->cache_size ){
> +
> +		if(!write_cd_buf(cd_header)) {

Space is needed here.

      	 	if (!write_cd_buf(cd_header)) {

> +			/*
> +			 * if enospc occurs in writing cd_header,
> +			 * fill the cd_header with zero and re-write it
> +			 * into the dumpfile.
> +			 */

Also, this comment is meangless, no additional information...

> +			memset(cd_header->buf, 0, cd_header->cache_size);
> +			write_cd_buf(cd_header);
> +
> +			return FALSE;
> +		}
> +
> +		if(!write_cd_buf(cd_page)) {
> +			/*
> +			 * if enospc occurs in writing cd_page,
> +			 * get how many pages having been written.
> +			 * and fill part of cd_header with zero according to it.
> +			 */
> +			written_pfns_size = sizeof(page_desc_t) *
> +					get_pfn_offset(cd_header->buf, cd_page);

Hmm, so get_pfn_offset() returns the number of pages? If so, this
function name of get_pfn_offset() is very confusing.

Is this variable name written_pfns_size good? That is, I associate
page frame with pfn, but this variable should represent size of page
headers.

> +
> +			memset(cd_header->buf, 0, cd_header->cache_size);
> +			cd_header->offset += written_pfns_size;
> +			cd_header->buf_size -= written_pfns_size;
> +			write_cd_buf(cd_header);
> +
> +			return FALSE;
> +		}
> +		cd_header->offset += cd_header->buf_size;
> +		cd_page->offset += cd_page->buf_size;
> +		cd_header->buf_size = 0;
> +		cd_page->buf_size = 0;
> +	}
> +
> +	write_cache(cd_header, pd, sizeof(page_desc_t));
> +	write_cache(cd_page, page_data, pd->size);
> +
> +	return TRUE;
> +}
> +
> +int
>  write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_page,
>  			 struct page_desc *pd_zero, off_t *offset_data, struct cycle *cycle)
>  {
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
--
Thanks.
HATAYAMA, Daisuke

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec