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

[George Dunlap <george.dunlap@eu.citrix.com>]

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<george.dunlap@eu.citrix.com>
> 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 = "<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
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>