Skip to contentSkip to conversation

libxl cleanup: distinguish filenames from sources

PATCH New George Dunlap 2 participants 6 messages May 15, 2012

lore
Share & tools
lore

George Dunlap

author
#0 lore raw

PATCH 1/1 libxl cleanup: distinguish filenames from sources

libxl cleanup: distinguish filenames from sources

author
#1 lore raw

PATCH 1/1 libxlu: Rename filename to config_source

The "filename" is a bit of a misnomer, as it's only used during error messages, and in most instances cases is actually set to "command line".

Rename it to "config_source" to make it clear that it's not used to actually open any files.

No functional changes.

Signed-off-by George Dunlap <[email protected]>
tools/libxl/libxlu_cfg.c +11 -11
diff -r 34a8cded4b39 -r 750b4dbab4b5 tools/libxl/libxlu_cfg.c
@@ -25,15 +25,15 @@
 #include "libxlu_cfg_l.h"
 #include "libxlu_cfg_i.h"
 
-XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename) {
+XLU_Config *xlu_cfg_init(FILE *report, const char *report_source) {
     XLU_Config *cfg;
 
     cfg= malloc(sizeof(*cfg));
     if (!cfg) return 0;
 
     cfg->report= report;
-    cfg->filename= strdup(report_filename);
-    if (!cfg->filename) { free(cfg); return 0; }
+    cfg->config_source= strdup(report_source);
+    if (!cfg->config_source) { free(cfg); return 0; }
 
     cfg->settings= 0;
     return cfg;

@@ -51,7 +51,7 @@ static int ctx_prep(CfgParseContext *ctx
     e= xlu__cfg_yylex_init_extra(ctx, &ctx->scanner);
     if (e) {
         fprintf(cfg->report,"%s: unable to create scanner: %s\n",
-                cfg->filename, strerror(e));
+                cfg->config_source, strerror(e));
         return e;
     }
     return 0;

@@ -117,7 +117,7 @@ int xlu_cfg_readdata(XLU_Config *cfg, co
     buf = xlu__cfg_yy_scan_bytes(data, length, ctx.scanner);
     if (!buf) {
         fprintf(cfg->report,"%s: unable to allocate scanner buffer\n",
-                cfg->filename);
+                cfg->config_source);
         ctx.err= ENOMEM;
         goto xe;
     }

@@ -151,7 +151,7 @@ void xlu_cfg_destroy(XLU_Config *cfg) {
         set_next= set->next;
         xlu__cfg_set_free(set);
     }
-    free(cfg->filename);
+    free(cfg->config_source);
     free(cfg);
 }
 

@@ -178,7 +178,7 @@ static int find_atom(const XLU_Config *c
             fprintf(cfg->report,
                     "%s:%d: warning: parameter `%s' is"
                     " a list but should be a single value\n",
-                    cfg->filename, set->lineno, n);
+                    cfg->config_source, set->lineno, n);
         return EINVAL;
     }
     *set_r= set;

@@ -223,14 +223,14 @@ int xlu_cfg_get_long(const XLU_Config *c
             fprintf(cfg->report,
                     "%s:%d: warning: parameter `%s' could not be parsed"
                     " as a number: %s\n",
-                    cfg->filename, set->lineno, n, strerror(e));
+                    cfg->config_source, set->lineno, n, strerror(e));
         return e;
     }
     if (*ep || ep==set->values[0]) {
         if (!dont_warn)
             fprintf(cfg->report,
                     "%s:%d: warning: parameter `%s' is not a valid number\n",
-                    cfg->filename, set->lineno, n);
+                    cfg->config_source, set->lineno, n);
         return EINVAL;
     }
     *value_r= l;

@@ -258,7 +258,7 @@ int xlu_cfg_get_list(const XLU_Config *c
             fprintf(cfg->report,
                     "%s:%d: warning: parameter `%s' is a single value"
                     " but should be a list\n",
-                    cfg->filename, set->lineno, n);
+                    cfg->config_source, set->lineno, n);
         }
         return EINVAL;
     }

@@ -467,7 +467,7 @@ void xlu__cfg_yyerror(YYLTYPE *loc, CfgP
 
     fprintf(ctx->cfg->report,
             "%s:%d: config parsing error near %s%.*s%s%s: %s\n",
-            ctx->cfg->filename, lineno,
+            ctx->cfg->config_source, lineno,
             len?"`":"", len, text, len?"'":"", newline,
             msg);
     if (!ctx->err) ctx->err= EINVAL;
tools/libxl/libxlu_disk.c +1 -1
diff -r 34a8cded4b39 -r 750b4dbab4b5 tools/libxl/libxlu_disk.c
@@ -10,7 +10,7 @@ void xlu__disk_err(DiskParseContext *dpc
             "%s: config parsing error in disk specification: %s"
             "%s%s%s"
             " in `%s'\n",
-            dpc->cfg->filename, message,
+            dpc->cfg->config_source, message,
             erroneous?": near `":"", erroneous?erroneous:"", erroneous?"'":"",
             dpc->spec);
     if (!dpc->err) dpc->err= EINVAL;
tools/libxl/libxlu_internal.h +1 -1
diff -r 34a8cded4b39 -r 750b4dbab4b5 tools/libxl/libxlu_internal.h
@@ -38,7 +38,7 @@ struct XLU_ConfigSetting { /* transparen
 struct XLU_Config {
     XLU_ConfigSetting *settings;
     FILE *report;
-    char *filename;
+    char *config_source;
 };
 
 typedef struct {
tools/libxl/libxlu_vif.c +1 -1
diff -r 34a8cded4b39 -r 750b4dbab4b5 tools/libxl/libxlu_vif.c
@@ -7,7 +7,7 @@ static const char *vif_internal_usec_re 
 static void xlu__vif_err(XLU_Config *cfg, const char *msg, const char *rate) {
     fprintf(cfg->report,
             "%s: config parsing error in vif: %s in `%s'\n",
-            cfg->filename, msg, rate);
+            cfg->config_source, msg, rate);
 }
 
 static int vif_parse_rate_bytes_per_sec(XLU_Config *cfg, const char *bytes,
author
#2 lore raw

PATCH 1/1 xl: Make clear distinction between "filename" and "data source"

In reply to George Dunlap

Many places in xl there's a variable or element named "filename" which does not contain a filename, but the source of the data for reporting purposes. Worse, there are variables which are sometimes actually used or interpreted as a filename depending on what other variables are set. This makes it difficult to tell when a string is purely cosmetic, and when another bit of code may actually attempt to call "open" with the string.

This patch makes a consistent distinction between "filename" (which always refers to the name of an actual file, and may be interpreted as such at some point) and "source" (which may be a filename, or may be another data source such as a migration stream or saved data).

This does add some variables and reshuffle where assignments happen; most notably, the "restore_filename" element of struct domain_create is now only set when restoring from a file.

But at a high level, there should be no funcitonal changes.

Signed-off-by George Dunlap <[email protected]>
tools/libxl/libxl_utils.c +3 -3
diff -r 750b4dbab4b5 -r d555dd6614af tools/libxl/libxl_utils.c
@@ -334,7 +334,7 @@ int libxl_read_file_contents(libxl_ctx *
                                                                           \
   int libxl_##rw##_exactly(libxl_ctx *ctx, int fd,                 \
                            constdata void *data, ssize_t sz,              \
-                           const char *filename, const char *what) {      \
+                           const char *source, const char *what) {      \
       ssize_t got;                                                        \
                                                                           \
       while (sz > 0) {                                                    \

@@ -343,7 +343,7 @@ int libxl_read_file_contents(libxl_ctx *
               if (errno == EINTR) continue;                               \
               if (!ctx) return errno;                                     \
               LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to " #rw " %s%s%s", \
-                           what?what:"", what?" from ":"", filename);     \
+                           what?what:"", what?" from ":"", source);       \
               return errno;                                               \
           }                                                               \
           if (got == 0) {                                                 \

@@ -352,7 +352,7 @@ int libxl_read_file_contents(libxl_ctx *
                      zero_is_eof                                          \
                      ? "file/stream truncated reading %s%s%s"             \
                      : "file/stream write returned 0! writing %s%s%s",    \
-                     what?what:"", what?" from ":"", filename);           \
+                     what?what:"", what?" from ":"", source);             \
               return EPROTO;                                              \
           }                                                               \
           sz -= got;                                                      \
tools/libxl/xl_cmdimpl.c +31 -25

179 lines not on this page · raw message

#3 lore raw

Re: [PATCH 2 of 2] xl: Make clear distinction between "filename" and "data source"

Quoted 13 lines · George Dunlap

Many places in xl there's a variable or element named "filename" which does not contain a filename, but the source of the data for reporting purposes. Worse, there are variables which are sometimes actually used or interpreted as a filename depending on what other variables are set. This makes it difficult to tell when a string is purely cosmetic, and when another bit of code may actually attempt to call "open" with the string.

This patch makes a consistent distinction between "filename" (which always refers to the name of an actual file, and may be interpreted as such at some point) and "source" (which may be a filename, or may be another data source such as a migration stream or saved data).

Quoted 7 lines

This does add some variables and reshuffle where assignments happen; most notably, the "restore_filename" element of struct domain_create is now only set when restoring from a file.

But at a high level, there should be no funcitonal changes.

Signed-off-by George Dunlap <[email protected]>

I've committed this (and the preceding patch), there were a few conflicts with Goncalo's vncviewer patch (a new param to parse_config_data). Please do check I've done the right thing.

Quoted 232 lines

diff -r 750b4dbab4b5 -r d555dd6614af tools/libxl/libxl_utils.c

tools/libxl/libxl_utils.c Tue May 15 15:51:25 2012 +0100 renamed from tools/libxl/libxl_utils.c Tue May 15 15:45:55 2012 +0100 +3 -3
@@ -334,7 +334,7 @@ int libxl_read_file_contents(libxl_ctx *
                                                                           \
   int libxl_##rw##_exactly(libxl_ctx *ctx, int fd,                 \
                            constdata void *data, ssize_t sz,              \
-                           const char *filename, const char *what) {      \
+                           const char *source, const char *what) {      \
       ssize_t got;                                                        \
                                                                           \
       while (sz > 0) {                                                    \

@@ -343,7 +343,7 @@ int libxl_read_file_contents(libxl_ctx *
               if (errno == EINTR) continue;                               \
               if (!ctx) return errno;                                     \
               LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to " #rw " %s%s%s", \
-                           what?what:"", what?" from ":"", filename);     \
+                           what?what:"", what?" from ":"", source);       \
               return errno;                                               \
           }                                                               \
           if (got == 0) {                                                 \

@@ -352,7 +352,7 @@ int libxl_read_file_contents(libxl_ctx *
                      zero_is_eof                                          \
                      ? "file/stream truncated reading %s%s%s"             \
                      : "file/stream write returned 0! writing %s%s%s",    \
-                     what?what:"", what?" from ":"", filename);           \
+                     what?what:"", what?" from ":"", source);             \
               return EPROTO;                                              \
           }                                                               \
           sz -= got;                                                      \

diff -r 750b4dbab4b5 -r d555dd6614af tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c Tue May 15 15:51:25 2012 +0100 renamed from tools/libxl/xl_cmdimpl.c Tue May 15 15:45:55 2012 +0100 +31 -25
@@ -520,9 +520,9 @@ vcpp_out:
     return rc;
 }
 
-static void parse_config_data(const char *configfile_filename_report,
-                              const char *configfile_data,
-                              int configfile_len,
+static void parse_config_data(const char *config_source,
+                              const char *config_data,
+                              int config_len,
                               libxl_domain_config *d_config)
 {
     const char *buf;

@@ -537,15 +537,15 @@ static void parse_config_data(const char
     libxl_domain_create_info *c_info = &d_config->c_info;
     libxl_domain_build_info *b_info = &d_config->b_info;
 
-    config= xlu_cfg_init(stderr, configfile_filename_report);
+    config= xlu_cfg_init(stderr, config_source);
     if (!config) {
         fprintf(stderr, "Failed to allocate for configuration\n");
         exit(1);
     }
 
-    e= xlu_cfg_readdata(config, configfile_data, configfile_len);
+    e= xlu_cfg_readdata(config, config_data, config_len);
     if (e) {
-        fprintf(stderr, "Failed to parse config file: %s\n", strerror(e));
+        fprintf(stderr, "Failed to parse config: %s\n", strerror(e));
         exit(1);
     }
 

@@ -1524,6 +1524,8 @@ static int create_domain(struct domain_c
     const char *config_file = dom_info->config_file;
     const char *extra_config = dom_info->extra_config;
     const char *restore_file = dom_info->restore_file;
+    const char *config_source = NULL;
+    const char *restore_source = NULL;
     int migrate_fd = dom_info->migrate_fd;
 
     int i;

@@ -1539,24 +1541,28 @@ static int create_domain(struct domain_c
     pid_t child_console_pid = -1;
     struct save_file_header hdr;
 
+    int restoring = (restore_file || (migrate_fd >= 0));
+
     libxl_domain_config_init(&d_config);
 
-    if (restore_file) {
+    if (restoring) {
         uint8_t *optdata_begin = 0;
         const uint8_t *optdata_here = 0;
         union { uint32_t u32; char b[4]; } u32buf;
         uint32_t badflags;
 
         if (migrate_fd >= 0) {
+            restore_source = "<incoming migration stream>";
             restore_fd = migrate_fd;
         } else {
+            restore_source = restore_file;
             restore_fd = open(restore_file, O_RDONLY);
             rc = libxl_fd_set_cloexec(ctx, restore_fd, 1);
             if (rc) return rc;
         }
 
         CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, &hdr,
-                   sizeof(hdr), restore_file, "header") );
+                   sizeof(hdr), restore_source, "header") );
         if (memcmp(hdr.magic, savefileheader_magic, sizeof(hdr.magic))) {
             fprintf(stderr, "File has wrong magic number -"
                     " corrupt or for a different tool?\n");

@@ -1569,7 +1575,7 @@ static int create_domain(struct domain_c
         fprintf(stderr, "Loading new save file %s"
                 " (new xl fmt info"
                 " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
-                restore_file, hdr.mandatory_flags, hdr.optional_flags,
+                restore_source, hdr.mandatory_flags, hdr.optional_flags,
                 hdr.optional_data_len);
 
         badflags = hdr.mandatory_flags & ~( 0 /* none understood yet */ );

@@ -1582,7 +1588,7 @@ static int create_domain(struct domain_c
         if (hdr.optional_data_len) {
             optdata_begin = xmalloc(hdr.optional_data_len);
             CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, optdata_begin,
-                   hdr.optional_data_len, restore_file, "optdata") );
+                   hdr.optional_data_len, restore_source, "optdata") );
         }
 
 #define OPTDATA_LEFT  (hdr.optional_data_len - (optdata_here - optdata_begin))

@@ -1617,7 +1623,7 @@ static int create_domain(struct domain_c
                                        &config_data, &config_len);
         if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
                            config_file, strerror(errno)); return ERROR_FAIL; }
-        if (!restore_file && extra_config && strlen(extra_config)) {
+        if (!restoring && extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
                 fprintf(stderr, "Failed to attach extra configration\n");
                 return ERROR_FAIL;

@@ -1632,19 +1638,20 @@ static int create_domain(struct domain_c
             config_len += sprintf(config_data + config_len, "\n%s\n",
                 extra_config);
         }
+        config_source=config_file;
     } else {
         if (!config_data) {
             fprintf(stderr, "Config file not specified and"
                     " none in save file\n");
             return ERROR_INVAL;
         }
-        config_file = "<saved>";
+        config_source = "<saved>";
     }
 
     if (!dom_info->quiet)
-        printf("Parsing config file %s\n", config_file);
-
-    parse_config_data(config_file, config_data, config_len, &d_config);
+        printf("Parsing config from %s\n", config_source);
+
+    parse_config_data(config_source, config_data, config_len, &d_config);
 
     if (migrate_fd >= 0) {
         if (d_config.c_info.name) {

@@ -1698,7 +1705,7 @@ start:
         autoconnect_console_how = 0;
     }
 
-    if ( restore_file ) {
+    if ( restoring ) {
         ret = libxl_domain_create_restore(ctx, &d_config,
                                           &domid, restore_fd,
                                           0, autoconnect_console_how);

@@ -2559,7 +2566,7 @@ static void list_domains_details(const l
 {
     libxl_domain_config d_config;
 
-    char *config_file;
+    char *config_source;
     uint8_t *data;
     int i, len, rc;
 

@@ -2570,13 +2577,13 @@ static void list_domains_details(const l
         rc = libxl_userdata_retrieve(ctx, info[i].domid, "xl", &data, &len);
         if (rc)
             continue;
-        CHK_ERRNO(asprintf(&config_file, "<domid %d data>", info[i].domid));
+        CHK_ERRNO(asprintf(&config_source, "<domid %d data>", info[i].domid));
         libxl_domain_config_init(&d_config);
-        parse_config_data(config_file, (char *)data, len, &d_config);
+        parse_config_data(config_source, (char *)data, len, &d_config);
         printf_info(default_output_format, info[i].domid, &d_config);
         libxl_domain_config_dispose(&d_config);
         free(data);
-        free(config_file);
+        free(config_source);
     }
 }
 

@@ -2679,7 +2686,7 @@ static void save_domain_core_begin(const
     }
 }
 
-static void save_domain_core_writeconfig(int fd, const char *filename,
+static void save_domain_core_writeconfig(int fd, const char *source,
                                   const uint8_t *config_data, int config_len)
 {
     struct save_file_header hdr;

@@ -2708,13 +2715,13 @@ static void save_domain_core_writeconfig
     /* that's the optional data */
 
     CHK_ERRNO( libxl_write_exactly(ctx, fd,
-        &hdr, sizeof(hdr), filename, "header") );
+        &hdr, sizeof(hdr), source, "header") );
     CHK_ERRNO( libxl_write_exactly(ctx, fd,
-        optdata_begin, hdr.optional_data_len, filename, "header") );
+        optdata_begin, hdr.optional_data_len, source, "header") );
 
     fprintf(stderr, "Saving to %s new xl format (info"
             " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
-            filename, hdr.mandatory_flags, hdr.optional_flags,
+            source, hdr.mandatory_flags, hdr.optional_flags,
             hdr.optional_data_len);
 }
 

@@ -3039,7 +3046,6 @@ static void migrate_receive(int debug, i
     dom_info.daemonize = daemonize;
     dom_info.monitor = monitor;
     dom_info.paused = 1;
-    dom_info.restore_file = "incoming migration stream";
     dom_info.migrate_fd = 0; /* stdin */
     dom_info.migration_domname_r = &migration_domname;
     dom_info.incr_generationid = 0;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

author
#4 lore raw

Re: [PATCH 2 of 2] xl: Make clear distinction between "filename" and "data source"

Quoted 18 lines · Ian Campbell
Quoted 17 lines · George Dunlap

Many places in xl there's a variable or element named "filename" which does not contain a filename, but the source of the data for reporting purposes. Worse, there are variables which are sometimes actually used or interpreted as a filename depending on what other variables are set. This makes it difficult to tell when a string is purely cosmetic, and when another bit of code may actually attempt to call "open" with the string.

This patch makes a consistent distinction between "filename" (which always refers to the name of an actual file, and may be interpreted as such at some point) and "source" (which may be a filename, or may be another data source such as a migration stream or saved data).

This does add some variables and reshuffle where assignments happen; most notably, the "restore_filename" element of struct domain_create is now only set when restoring from a file.

Quoted 6 lines
Quoted 3 lines

But at a high level, there should be no funcitonal changes.

Signed-off-by George Dunlap<[email protected]>

I've committed this (and the preceding patch), there were a few conflicts with Goncalo's vncviewer patch (a new param to parse_config_data). Please do check I've done the right thing.

Looks like you missed one s/config_file/config_source/ for parse_config_data; I'll follow-up with a patch.

-George

Quoted 233 lines
Quoted 232 lines

diff -r 750b4dbab4b5 -r d555dd6614af tools/libxl/libxl_utils.c

tools/libxl/libxl_utils.c Tue May 15 15:51:25 2012 +0100 renamed from tools/libxl/libxl_utils.c Tue May 15 15:45:55 2012 +0100 +3 -3
@@ -334,7 +334,7 @@ int libxl_read_file_contents(libxl_ctx *
                                                                            \
    int libxl_##rw##_exactly(libxl_ctx *ctx, int fd,                 \
                             constdata void *data, ssize_t sz,              \
-                           const char *filename, const char *what) {      \
+                           const char *source, const char *what) {      \
        ssize_t got;                                                        \
                                                                            \
        while (sz>  0) {                                                    \

@@ -343,7 +343,7 @@ int libxl_read_file_contents(libxl_ctx *
                if (errno == EINTR) continue;                               \
                if (!ctx) return errno;                                     \
                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to " #rw " %s%s%s", \
-                           what?what:"", what?" from ":"", filename);     \
+                           what?what:"", what?" from ":"", source);       \
                return errno;                                               \
            }                                                               \
            if (got == 0) {                                                 \

@@ -352,7 +352,7 @@ int libxl_read_file_contents(libxl_ctx *
                       zero_is_eof                                          \
                       ? "file/stream truncated reading %s%s%s"             \
                       : "file/stream write returned 0! writing %s%s%s",    \
-                     what?what:"", what?" from ":"", filename);           \
+                     what?what:"", what?" from ":"", source);             \
                return EPROTO;                                              \
            }                                                               \
            sz -= got;                                                      \

diff -r 750b4dbab4b5 -r d555dd6614af tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c Tue May 15 15:51:25 2012 +0100 renamed from tools/libxl/xl_cmdimpl.c Tue May 15 15:45:55 2012 +0100 +31 -25
@@ -520,9 +520,9 @@ vcpp_out:
      return rc;
  }
 
-static void parse_config_data(const char *configfile_filename_report,
-                              const char *configfile_data,
-                              int configfile_len,
+static void parse_config_data(const char *config_source,
+                              const char *config_data,
+                              int config_len,
                                libxl_domain_config *d_config)
  {
      const char *buf;

@@ -537,15 +537,15 @@ static void parse_config_data(const char
      libxl_domain_create_info *c_info =&d_config->c_info;
      libxl_domain_build_info *b_info =&d_config->b_info;
 
-    config= xlu_cfg_init(stderr, configfile_filename_report);
+    config= xlu_cfg_init(stderr, config_source);
      if (!config) {
          fprintf(stderr, "Failed to allocate for configuration\n");
          exit(1);
      }
 
-    e= xlu_cfg_readdata(config, configfile_data, configfile_len);
+    e= xlu_cfg_readdata(config, config_data, config_len);
      if (e) {
-        fprintf(stderr, "Failed to parse config file: %s\n", strerror(e));
+        fprintf(stderr, "Failed to parse config: %s\n", strerror(e));
          exit(1);
      }
 

@@ -1524,6 +1524,8 @@ static int create_domain(struct domain_c
      const char *config_file = dom_info->config_file;
      const char *extra_config = dom_info->extra_config;
      const char *restore_file = dom_info->restore_file;
+    const char *config_source = NULL;
+    const char *restore_source = NULL;
      int migrate_fd = dom_info->migrate_fd;
 
      int i;

@@ -1539,24 +1541,28 @@ static int create_domain(struct domain_c
      pid_t child_console_pid = -1;
      struct save_file_header hdr;
 
+    int restoring = (restore_file || (migrate_fd>= 0));
+
      libxl_domain_config_init(&d_config);
 
-    if (restore_file) {
+    if (restoring) {
          uint8_t *optdata_begin = 0;
          const uint8_t *optdata_here = 0;
          union { uint32_t u32; char b[4]; } u32buf;
          uint32_t badflags;
 
          if (migrate_fd>= 0) {
+            restore_source = "<incoming migration stream>";
              restore_fd = migrate_fd;
          } else {
+            restore_source = restore_file;
              restore_fd = open(restore_file, O_RDONLY);
              rc = libxl_fd_set_cloexec(ctx, restore_fd, 1);
              if (rc) return rc;
          }
 
          CHK_ERRNO( libxl_read_exactly(ctx, restore_fd,&hdr,
-                   sizeof(hdr), restore_file, "header") );
+                   sizeof(hdr), restore_source, "header") );
          if (memcmp(hdr.magic, savefileheader_magic, sizeof(hdr.magic))) {
              fprintf(stderr, "File has wrong magic number -"
                      " corrupt or for a different tool?\n");

@@ -1569,7 +1575,7 @@ static int create_domain(struct domain_c
          fprintf(stderr, "Loading new save file %s"
                  " (new xl fmt info"
                  " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
-                restore_file, hdr.mandatory_flags, hdr.optional_flags,
+                restore_source, hdr.mandatory_flags, hdr.optional_flags,
                  hdr.optional_data_len);
 
          badflags = hdr.mandatory_flags&  ~( 0 /* none understood yet */ );

@@ -1582,7 +1588,7 @@ static int create_domain(struct domain_c
          if (hdr.optional_data_len) {
              optdata_begin = xmalloc(hdr.optional_data_len);
              CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, optdata_begin,
-                   hdr.optional_data_len, restore_file, "optdata") );
+                   hdr.optional_data_len, restore_source, "optdata") );
          }
 
  #define OPTDATA_LEFT  (hdr.optional_data_len - (optdata_here - optdata_begin))

@@ -1617,7 +1623,7 @@ static int create_domain(struct domain_c
                                         &config_data,&config_len);
          if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
                             config_file, strerror(errno)); return ERROR_FAIL; }
-        if (!restore_file&&  extra_config&&  strlen(extra_config)) {
+        if (!restoring&&  extra_config&&  strlen(extra_config)) {
              if (config_len>  INT_MAX - (strlen(extra_config) + 2 + 1)) {
                  fprintf(stderr, "Failed to attach extra configration\n");
                  return ERROR_FAIL;

@@ -1632,19 +1638,20 @@ static int create_domain(struct domain_c
              config_len += sprintf(config_data + config_len, "\n%s\n",
                  extra_config);
          }
+        config_source=config_file;
      } else {
          if (!config_data) {
              fprintf(stderr, "Config file not specified and"
                      " none in save file\n");
              return ERROR_INVAL;
          }
-        config_file = "<saved>";
+        config_source = "<saved>";
      }
 
      if (!dom_info->quiet)
-        printf("Parsing config file %s\n", config_file);
-
-    parse_config_data(config_file, config_data, config_len,&d_config);
+        printf("Parsing config from %s\n", config_source);
+
+    parse_config_data(config_source, config_data, config_len,&d_config);
 
      if (migrate_fd>= 0) {
          if (d_config.c_info.name) {

@@ -1698,7 +1705,7 @@ start:
          autoconnect_console_how = 0;
      }
 
-    if ( restore_file ) {
+    if ( restoring ) {
          ret = libxl_domain_create_restore(ctx,&d_config,
                                            &domid, restore_fd,
                                            0, autoconnect_console_how);

@@ -2559,7 +2566,7 @@ static void list_domains_details(const l
  {
      libxl_domain_config d_config;
 
-    char *config_file;
+    char *config_source;
      uint8_t *data;
      int i, len, rc;
 

@@ -2570,13 +2577,13 @@ static void list_domains_details(const l
          rc = libxl_userdata_retrieve(ctx, info[i].domid, "xl",&data,&len);
          if (rc)
              continue;
-        CHK_ERRNO(asprintf(&config_file, "<domid %d data>", info[i].domid));
+        CHK_ERRNO(asprintf(&config_source, "<domid %d data>", info[i].domid));
          libxl_domain_config_init(&d_config);
-        parse_config_data(config_file, (char *)data, len,&d_config);
+        parse_config_data(config_source, (char *)data, len,&d_config);
          printf_info(default_output_format, info[i].domid,&d_config);
          libxl_domain_config_dispose(&d_config);
          free(data);
-        free(config_file);
+        free(config_source);
      }
  }
 

@@ -2679,7 +2686,7 @@ static void save_domain_core_begin(const
      }
  }
 
-static void save_domain_core_writeconfig(int fd, const char *filename,
+static void save_domain_core_writeconfig(int fd, const char *source,
                                    const uint8_t *config_data, int config_len)
  {
      struct save_file_header hdr;

@@ -2708,13 +2715,13 @@ static void save_domain_core_writeconfig
      /* that's the optional data */
 
      CHK_ERRNO( libxl_write_exactly(ctx, fd,
-&hdr, sizeof(hdr), filename, "header") );
+&hdr, sizeof(hdr), source, "header") );
      CHK_ERRNO( libxl_write_exactly(ctx, fd,
-        optdata_begin, hdr.optional_data_len, filename, "header") );
+        optdata_begin, hdr.optional_data_len, source, "header") );
 
      fprintf(stderr, "Saving to %s new xl format (info"
              " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
-            filename, hdr.mandatory_flags, hdr.optional_flags,
+            source, hdr.mandatory_flags, hdr.optional_flags,
              hdr.optional_data_len);
  }
 

@@ -3039,7 +3046,6 @@ static void migrate_receive(int debug, i
      dom_info.daemonize = daemonize;
      dom_info.monitor = monitor;
      dom_info.paused = 1;
-    dom_info.restore_file = "incoming migration stream";
      dom_info.migrate_fd = 0; /* stdin */
      dom_info.migration_domname_r =&migration_domname;
      dom_info.incr_generationid = 0;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#5 lore raw

Re: [PATCH 2 of 2] xl: Make clear distinction between "filename" and "data source"

Quoted 19 lines · George Dunlap
Quoted 18 lines · Ian Campbell
Quoted 17 lines · George Dunlap

Many places in xl there's a variable or element named "filename" which does not contain a filename, but the source of the data for reporting purposes. Worse, there are variables which are sometimes actually used or interpreted as a filename depending on what other variables are set. This makes it difficult to tell when a string is purely cosmetic, and when another bit of code may actually attempt to call "open" with the string.

This patch makes a consistent distinction between "filename" (which always refers to the name of an actual file, and may be interpreted as such at some point) and "source" (which may be a filename, or may be another data source such as a migration stream or saved data).

This does add some variables and reshuffle where assignments happen; most notably, the "restore_filename" element of struct domain_create is now only set when restoring from a file.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

But at a high level, there should be no funcitonal changes.

Signed-off-by George Dunlap<[email protected]>

I've committed this (and the preceding patch), there were a few conflicts with Goncalo's vncviewer patch (a new param to parse_config_data). Please do check I've done the right thing.

Looks like you missed one s/config_file/config_source/ for parse_config_data; I'll follow-up with a patch.

Thanks, I did build test but I guess this is a runtime issue?

Quoted 236 lines

-George

Quoted 233 lines
Quoted 232 lines

diff -r 750b4dbab4b5 -r d555dd6614af tools/libxl/libxl_utils.c

tools/libxl/libxl_utils.c Tue May 15 15:51:25 2012 +0100 renamed from tools/libxl/libxl_utils.c Tue May 15 15:45:55 2012 +0100 +3 -3
@@ -334,7 +334,7 @@ int libxl_read_file_contents(libxl_ctx *
                                                                            \
    int libxl_##rw##_exactly(libxl_ctx *ctx, int fd,                 \
                             constdata void *data, ssize_t sz,              \
-                           const char *filename, const char *what) {      \
+                           const char *source, const char *what) {      \
        ssize_t got;                                                        \
                                                                            \
        while (sz>  0) {                                                    \

@@ -343,7 +343,7 @@ int libxl_read_file_contents(libxl_ctx *
                if (errno == EINTR) continue;                               \
                if (!ctx) return errno;                                     \
                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to " #rw " %s%s%s", \
-                           what?what:"", what?" from ":"", filename);     \
+                           what?what:"", what?" from ":"", source);       \
                return errno;                                               \
            }                                                               \
            if (got == 0) {                                                 \

@@ -352,7 +352,7 @@ int libxl_read_file_contents(libxl_ctx *
                       zero_is_eof                                          \
                       ? "file/stream truncated reading %s%s%s"             \
                       : "file/stream write returned 0! writing %s%s%s",    \
-                     what?what:"", what?" from ":"", filename);           \
+                     what?what:"", what?" from ":"", source);             \
                return EPROTO;                                              \
            }                                                               \
            sz -= got;                                                      \

diff -r 750b4dbab4b5 -r d555dd6614af tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c Tue May 15 15:51:25 2012 +0100 renamed from tools/libxl/xl_cmdimpl.c Tue May 15 15:45:55 2012 +0100 +31 -25
@@ -520,9 +520,9 @@ vcpp_out:
      return rc;
  }
 
-static void parse_config_data(const char *configfile_filename_report,
-                              const char *configfile_data,
-                              int configfile_len,
+static void parse_config_data(const char *config_source,
+                              const char *config_data,
+                              int config_len,
                                libxl_domain_config *d_config)
  {
      const char *buf;

@@ -537,15 +537,15 @@ static void parse_config_data(const char
      libxl_domain_create_info *c_info =&d_config->c_info;
      libxl_domain_build_info *b_info =&d_config->b_info;
 
-    config= xlu_cfg_init(stderr, configfile_filename_report);
+    config= xlu_cfg_init(stderr, config_source);
      if (!config) {
          fprintf(stderr, "Failed to allocate for configuration\n");
          exit(1);
      }
 
-    e= xlu_cfg_readdata(config, configfile_data, configfile_len);
+    e= xlu_cfg_readdata(config, config_data, config_len);
      if (e) {
-        fprintf(stderr, "Failed to parse config file: %s\n", strerror(e));
+        fprintf(stderr, "Failed to parse config: %s\n", strerror(e));
          exit(1);
      }
 

@@ -1524,6 +1524,8 @@ static int create_domain(struct domain_c
      const char *config_file = dom_info->config_file;
      const char *extra_config = dom_info->extra_config;
      const char *restore_file = dom_info->restore_file;
+    const char *config_source = NULL;
+    const char *restore_source = NULL;
      int migrate_fd = dom_info->migrate_fd;
 
      int i;

@@ -1539,24 +1541,28 @@ static int create_domain(struct domain_c
      pid_t child_console_pid = -1;
      struct save_file_header hdr;
 
+    int restoring = (restore_file || (migrate_fd>= 0));
+
      libxl_domain_config_init(&d_config);
 
-    if (restore_file) {
+    if (restoring) {
          uint8_t *optdata_begin = 0;
          const uint8_t *optdata_here = 0;
          union { uint32_t u32; char b[4]; } u32buf;
          uint32_t badflags;
 
          if (migrate_fd>= 0) {
+            restore_source = "<incoming migration stream>";
              restore_fd = migrate_fd;
          } else {
+            restore_source = restore_file;
              restore_fd = open(restore_file, O_RDONLY);
              rc = libxl_fd_set_cloexec(ctx, restore_fd, 1);
              if (rc) return rc;
          }
 
          CHK_ERRNO( libxl_read_exactly(ctx, restore_fd,&hdr,
-                   sizeof(hdr), restore_file, "header") );
+                   sizeof(hdr), restore_source, "header") );
          if (memcmp(hdr.magic, savefileheader_magic, sizeof(hdr.magic))) {
              fprintf(stderr, "File has wrong magic number -"
                      " corrupt or for a different tool?\n");

@@ -1569,7 +1575,7 @@ static int create_domain(struct domain_c
          fprintf(stderr, "Loading new save file %s"
                  " (new xl fmt info"
                  " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
-                restore_file, hdr.mandatory_flags, hdr.optional_flags,
+                restore_source, hdr.mandatory_flags, hdr.optional_flags,
                  hdr.optional_data_len);
 
          badflags = hdr.mandatory_flags&  ~( 0 /* none understood yet */ );

@@ -1582,7 +1588,7 @@ static int create_domain(struct domain_c
          if (hdr.optional_data_len) {
              optdata_begin = xmalloc(hdr.optional_data_len);
              CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, optdata_begin,
-                   hdr.optional_data_len, restore_file, "optdata") );
+                   hdr.optional_data_len, restore_source, "optdata") );
          }
 
  #define OPTDATA_LEFT  (hdr.optional_data_len - (optdata_here - optdata_begin))

@@ -1617,7 +1623,7 @@ static int create_domain(struct domain_c
                                         &config_data,&config_len);
          if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
                             config_file, strerror(errno)); return ERROR_FAIL; }
-        if (!restore_file&&  extra_config&&  strlen(extra_config)) {
+        if (!restoring&&  extra_config&&  strlen(extra_config)) {
              if (config_len>  INT_MAX - (strlen(extra_config) + 2 + 1)) {
                  fprintf(stderr, "Failed to attach extra configration\n");
                  return ERROR_FAIL;

@@ -1632,19 +1638,20 @@ static int create_domain(struct domain_c
              config_len += sprintf(config_data + config_len, "\n%s\n",
                  extra_config);
          }
+        config_source=config_file;
      } else {
          if (!config_data) {
              fprintf(stderr, "Config file not specified and"
                      " none in save file\n");
              return ERROR_INVAL;
          }
-        config_file = "<saved>";
+        config_source = "<saved>";
      }
 
      if (!dom_info->quiet)
-        printf("Parsing config file %s\n", config_file);
-
-    parse_config_data(config_file, config_data, config_len,&d_config);
+        printf("Parsing config from %s\n", config_source);
+
+    parse_config_data(config_source, config_data, config_len,&d_config);
 
      if (migrate_fd>= 0) {
          if (d_config.c_info.name) {

@@ -1698,7 +1705,7 @@ start:
          autoconnect_console_how = 0;
      }
 
-    if ( restore_file ) {
+    if ( restoring ) {
          ret = libxl_domain_create_restore(ctx,&d_config,
                                            &domid, restore_fd,
                                            0, autoconnect_console_how);

@@ -2559,7 +2566,7 @@ static void list_domains_details(const l
  {
      libxl_domain_config d_config;
 
-    char *config_file;
+    char *config_source;
      uint8_t *data;
      int i, len, rc;
 

@@ -2570,13 +2577,13 @@ static void list_domains_details(const l
          rc = libxl_userdata_retrieve(ctx, info[i].domid, "xl",&data,&len);
          if (rc)
              continue;
-        CHK_ERRNO(asprintf(&config_file, "<domid %d data>", info[i].domid));
+        CHK_ERRNO(asprintf(&config_source, "<domid %d data>", info[i].domid));
          libxl_domain_config_init(&d_config);
-        parse_config_data(config_file, (char *)data, len,&d_config);
+        parse_config_data(config_source, (char *)data, len,&d_config);
          printf_info(default_output_format, info[i].domid,&d_config);
          libxl_domain_config_dispose(&d_config);
          free(data);
-        free(config_file);
+        free(config_source);
      }
  }
 

@@ -2679,7 +2686,7 @@ static void save_domain_core_begin(const
      }
  }
 
-static void save_domain_core_writeconfig(int fd, const char *filename,
+static void save_domain_core_writeconfig(int fd, const char *source,
                                    const uint8_t *config_data, int config_len)
  {
      struct save_file_header hdr;

@@ -2708,13 +2715,13 @@ static void save_domain_core_writeconfig
      /* that's the optional data */
 
      CHK_ERRNO( libxl_write_exactly(ctx, fd,
-&hdr, sizeof(hdr), filename, "header") );
+&hdr, sizeof(hdr), source, "header") );
      CHK_ERRNO( libxl_write_exactly(ctx, fd,
-        optdata_begin, hdr.optional_data_len, filename, "header") );
+        optdata_begin, hdr.optional_data_len, source, "header") );
 
      fprintf(stderr, "Saving to %s new xl format (info"
              " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
-            filename, hdr.mandatory_flags, hdr.optional_flags,
+            source, hdr.mandatory_flags, hdr.optional_flags,
              hdr.optional_data_len);
  }
 

@@ -3039,7 +3046,6 @@ static void migrate_receive(int debug, i
      dom_info.daemonize = daemonize;
      dom_info.monitor = monitor;
      dom_info.paused = 1;
-    dom_info.restore_file = "incoming migration stream";
      dom_info.migrate_fd = 0; /* stdin */
      dom_info.migration_domname_r =&migration_domname;
      dom_info.incr_generationid = 0;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel