From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 2 of 2] xl: Make clear distinction between "filename" and "data source" Date: Tue, 15 May 2012 16:47:00 +0100 Message-ID: <4FB27A74.3070605@eu.citrix.com> References: <1337095764.19852.30.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1337095764.19852.30.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 15/05/12 16:29, Ian Campbell wrote: > On Tue, 2012-05-15 at 15:51 +0100, George Dunlap wrote: >> 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 > 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 > >> diff -r 750b4dbab4b5 -r d555dd6614af tools/libxl/libxl_utils.c >> --- a/tools/libxl/libxl_utils.c Tue May 15 15:45:55 2012 +0100 >> +++ b/tools/libxl/libxl_utils.c Tue May 15 15:51:25 2012 +0100 >> @@ -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 >> --- a/tools/libxl/xl_cmdimpl.c Tue May 15 15:45:55 2012 +0100 >> +++ b/tools/libxl/xl_cmdimpl.c Tue May 15 15:51:25 2012 +0100 >> @@ -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 = ""; >> 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 = ""; >> + config_source = ""; >> } >> >> 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, "", info[i].domid)); >> + CHK_ERRNO(asprintf(&config_source, "", 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 >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel >