Xen devel - Sep 2006 - Re: dumpcore changes -- [Xen-changelog] [xen-unstable] In this patch, the xc_domain_dumpcore_via_callback() in xc_core.c of

This change has the effect of adding some complexity to the callback
routines. The original callback passed an opaque argument which was a
private item for the use of the controlling mechanism and its callback
function.  This change removes this and specifies only an fd.  While
it''s
possible for the controlling mechanism to use the fd as an index to find
internal data structures, this is a bit more work and it''s not clear to
me
that it makes the interface more usable. You could also easily image
controlling mechanisms that did not use an fd, in which case this would
simply be an "index".

This also changes the user-mode programs that might call this library.
While such changes sometimes cannot be avoided, it''s certainly
appealing to
retain APIs where possible. (The comment should also be cleaned up)

Was there a specific reason for the interface change ?  If so, knowing more
about it would help my understanding of this change.

Thanks !


  * and passes an opaque object for the use of the function and
  * created by the caller of xc_domain_dumpcore_via_callback.
  */
-typedef int (dumpcore_rtn_t)(void *arg, char *buffer, unsigned int length);
+typedef int (dumpcore_rtn_t)(int fd, char *buffer, unsigned int length);




On 9/17/06, Xen patchbot-unstable <patchbot-unstable@lists.xensource.com>
wrote:
>
> # HG changeset patch
> # User root@procyon
> # Node ID 11645dda144c3c8365dd2a6a64cb5a7d7da01170
> # Parent  30c659b655f15da7f6eeca68cd204162fbcf4733
> In this patch, the xc_domain_dumpcore_via_callback() in xc_core.c of
> libxc is modified. Previously, the xc_domain_dumpcore_via_callback()
> did not respond to error when copy_from_domain_page() failed.
> In other words, the dump core remained silent even if mapping the domain
> memory failed and its page could not be copied. When this happened,
> erroneous data had been dumped to the file without the user realizing it.
> Now, it has been modified so that if copy_from_domain_page fails,
> this fact is recorded in the logfile and make imcoplerte-list.
> However even in such cases, the dumping will continue as before.
>
> Signed-off-by: Ken Hironaka <hironaka.ken@soft.fujitsu.com>
> Signed-off-by: Akio Takebe <takebe_akio@jp.fujitsu.com>
> ---
> tools/libxc/xc_core.c |   87
> +++++++++++++++++++++++++++++++++++---------------
> tools/libxc/xenctrl.h |    2 -
> 2 files changed, 63 insertions(+), 26 deletions(-)
>
> diff -r 30c659b655f1 -r 11645dda144c tools/libxc/xc_core.c
> --- a/tools/libxc/xc_core.c     Thu Sep 14 08:19:39 2006 +0100
> +++ b/tools/libxc/xc_core.c     Thu Sep 14 08:19:41 2006 +0100
> @@ -5,6 +5,12 @@
> /* number of pages to write at a time */
> #define DUMP_INCREMENT (4 * 1024)
> #define round_pgup(_p)    (((_p)+(PAGE_SIZE-1))&PAGE_MASK)
> +
> +/* Callback args for writing to a local dump file. */
> +struct dump_args {
> +    int     fd;
> +    int     incomp_fd;
> +};
>
> static int
> copy_from_domain_page(int xc_handle,
> @@ -27,7 +33,7 @@ xc_domain_dumpcore_via_callback(int xc_h
>                                  void *args,
>                                  dumpcore_rtn_t dump_rtn)
> {
> -    unsigned long nr_pages;
> +    unsigned long n, nr_pages;
>      xen_pfn_t *page_array = NULL;
>      xc_dominfo_t info;
>      int i, nr_vcpus = 0;
> @@ -37,6 +43,12 @@ xc_domain_dumpcore_via_callback(int xc_h
>      char dummy[PAGE_SIZE];
>      int dummy_len;
>      int sts;
> +    unsigned int cpy_err_cnt = 0;
> +    struct dump_args *da = args;
> +    int fd = da->fd;
> +    int incomp_fd = da->incomp_fd;
> +    char cpy_err_mesg[64];
> +    int mesg_bytes;
>
>      if ( (dump_mem_start = malloc(DUMP_INCREMENT*PAGE_SIZE)) == NULL )
>      {
> @@ -73,11 +85,11 @@ xc_domain_dumpcore_via_callback(int xc_h
>                   (nr_pages * sizeof(xen_pfn_t)));
>      header.xch_pages_offset = round_pgup(dummy_len);
>
> -    sts = dump_rtn(args, (char *)&header, sizeof(struct
xc_core_header));
> -    if ( sts != 0 )
> -        goto error_out;
> -
> -    sts = dump_rtn(args, (char *)&ctxt, sizeof(ctxt[0]) * nr_vcpus);
> +    sts = dump_rtn(fd, (char *)&header, sizeof(struct
xc_core_header));
> +    if ( sts != 0 )
> +        goto error_out;
> +
> +    sts = dump_rtn(fd, (char *)&ctxt, sizeof(ctxt[0]) * nr_vcpus);
>      if ( sts != 0 )
>          goto error_out;
>
> @@ -91,27 +103,39 @@ xc_domain_dumpcore_via_callback(int xc_h
>          IPRINTF("Could not get the page frame list\n");
>          goto error_out;
>      }
> -    sts = dump_rtn(args, (char *)page_array, nr_pages *
> sizeof(xen_pfn_t));
> +    sts = dump_rtn(fd, (char *)page_array, nr_pages * sizeof(xen_pfn_t));
>      if ( sts != 0 )
>          goto error_out;
>
>      /* Pad the output data to page alignment. */
>      memset(dummy, 0, PAGE_SIZE);
> -    sts = dump_rtn(args, dummy, header.xch_pages_offset - dummy_len);
> -    if ( sts != 0 )
> -        goto error_out;
> -
> -    for ( dump_mem = dump_mem_start, i = 0; i < nr_pages; i++ )
> -    {
> -        copy_from_domain_page(xc_handle, domid, page_array[i], dump_mem);
> +    sts = dump_rtn(fd, dummy, header.xch_pages_offset - dummy_len);
> +    if ( sts != 0 )
> +        goto error_out;
> +
> +    for ( dump_mem = dump_mem_start, n = 0; n < nr_pages; n++ )
> +    {
> +        sts = copy_from_domain_page(xc_handle, domid, page_array[i],
> dump_mem);
> +        if( sts != 0 ){
> +            memset(dump_mem, 0, PAGE_SIZE);
> +            cpy_err_cnt++;
> +            memset(cpy_err_mesg, 0, sizeof(cpy_err_mesg));
> +            mesg_bytes = sprintf(cpy_err_mesg, "Cannot
> copy_from_domain_page (%lu)\n", n);
> +            dump_rtn(incomp_fd, (char *)cpy_err_mesg, mesg_bytes);
> +        }
> +
>          dump_mem += PAGE_SIZE;
> -        if ( ((i + 1) % DUMP_INCREMENT == 0) || ((i + 1) == nr_pages) )
> +        if ( ((n + 1) % DUMP_INCREMENT == 0) || ((n + 1) == nr_pages) )
>          {
> -            sts = dump_rtn(args, dump_mem_start, dump_mem -
> dump_mem_start);
> +            sts = dump_rtn(fd, dump_mem_start, dump_mem -
> dump_mem_start);
>              if ( sts != 0 )
>                  goto error_out;
>              dump_mem = dump_mem_start;
>          }
> +    }
> +    if( cpy_err_cnt != 0 ){
> +        IPRINTF("Could not copy from domid=%d (%d)pages\n",
domid,
> cpy_err_cnt);
> +        goto error_out;
>      }
>
>      free(dump_mem_start);
> @@ -124,20 +148,14 @@ xc_domain_dumpcore_via_callback(int xc_h
>      return -1;
> }
>
> -/* Callback args for writing to a local dump file. */
> -struct dump_args {
> -    int     fd;
> -};
> -
> /* Callback routine for writing to a local dump file. */
> -static int local_file_dump(void *args, char *buffer, unsigned int length)
> -{
> -    struct dump_args *da = args;
> +static int local_file_dump(int fd, char *buffer, unsigned int length)
> +{
>      int bytes, offset;
>
>      for ( offset = 0; offset < length; offset += bytes )
>      {
> -        bytes = write(da->fd, &buffer[offset], length-offset);
> +        bytes = write(fd, &buffer[offset], length-offset);
>          if ( bytes <= 0 )
>          {
>              PERROR("Failed to write buffer: %s",
strerror(errno));
> @@ -154,11 +172,26 @@ xc_domain_dumpcore(int xc_handle,
>                     const char *corename)
> {
>      struct dump_args da;
> +    char *incomp_file;
>      int sts;
>
>      if ( (da.fd = open(corename, O_CREAT|O_RDWR, S_IWUSR|S_IRUSR)) < 0
)
>      {
>          PERROR("Could not open corefile %s: %s", corename,
> strerror(errno));
> +        return -errno;
> +    }
> +
> +
> +    if ( (incomp_file = (char *)malloc(sizeof(corename) + 12)) == NULL )
> +    {
> +        PERROR("Could not allocate incomp_file");
> +        return -errno;
> +    }
> +
> +    sprintf(incomp_file, "%s-incomp.list", corename);
> +    if ( (da.incomp_fd = open(incomp_file, O_CREAT|O_RDWR,
> S_IWUSR|S_IRUSR)) < 0 )
> +    {
> +        PERROR("Could not open corefile %s: %s", incomp_file,
> strerror(errno));
>          return -errno;
>      }
>
> @@ -166,6 +199,10 @@ xc_domain_dumpcore(int xc_handle,
>          xc_handle, domid, &da, &local_file_dump);
>
>      close(da.fd);
> +    close(da.incomp_fd);
> +
> +    if( sts == 0)
> +        unlink(incomp_file);
>
>      return sts;
> }
> diff -r 30c659b655f1 -r 11645dda144c tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h     Thu Sep 14 08:19:39 2006 +0100
> +++ b/tools/libxc/xenctrl.h     Thu Sep 14 08:19:41 2006 +0100
> @@ -166,7 +166,7 @@ int xc_domain_dumpcore(int xc_handle,
>   * and passes an opaque object for the use of the function and
>   * created by the caller of xc_domain_dumpcore_via_callback.
>   */
> -typedef int (dumpcore_rtn_t)(void *arg, char *buffer, unsigned int
> length);
> +typedef int (dumpcore_rtn_t)(int fd, char *buffer, unsigned int length);
>
> int xc_domain_dumpcore_via_callback(int xc_handle,
>                                      uint32_t domid,
>
> _______________________________________________
> Xen-changelog mailing list
> Xen-changelog@lists.xensource.com
> http://lists.xensource.com/xen-changelog
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Yuk. This patch should never have been applied. It¹s certainly clearly never
been tested (e.g., because the iteration variable was changed from Œi¹ to
Œn¹ the copy_from_domain_page() now always copies from the wrong page). Even
if it did work, the interface change is more than a little questionable.

I¹ll revert the patch. Thanks for flagging the issue.

 -- Keir

On 18/9/06 13:39, "B Thomas" <bjthomas3@gmail.com> wrote:
> This change has the effect of adding some complexity to the callback
routines.
> The original callback passed an opaque argument which was a private item
for
> the use of the controlling mechanism and its callback function.  This
change
> removes this and specifies only an fd.  While it''s possible for
the
> controlling mechanism to use the fd as an index to find internal data
> structures, this is a bit more work and it''s not clear to me that
it makes the
> interface more usable. You could also easily image controlling mechanisms
that
> did not use an fd, in which case this would simply be an "index".
> 
> This also changes the user-mode programs that might call this library. 
While
> such changes sometimes cannot be avoided, it''s certainly appealing
to retain
> APIs where possible. (The comment should also be cleaned up)
> 
> Was there a specific reason for the interface change ?  If so, knowing more
> about it would help my understanding of this change.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel