Missed cc qemu-devel, added CC, sorry.
-------- 原始消息 --------
主题: Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter
日期: Fri, 21 Sep 2012 10:43:12 +0800
发件人: Dong Xu Wang <wdon...@vnet.linux.ibm.com>
收件人: Markus Armbruster <arm...@redhat.com>
于 9/20/2012 5:16 PM, Markus Armbruster 写道:
> Dong Xu Wang <wdon...@linux.vnet.ibm.com> writes:
>
>> Markus, I am working with v2 and have some questions based your comments.
>
> Your replies are very hard to read, because whatever you use to send
> them wraps quoted lines. Please fix that.
>
Sorry for that, will notice next time.
>> On Fri, Sep 7, 2012 at 4:42 PM, Markus Armbruster <arm...@redhat.com> wrote:
>>> Some overlap with what I'm working on, but since you have code to show,
>>> and I don't, I'll review yours as if mine didn't exist.
>>>
>>>
>>> Dong Xu Wang <wdon...@linux.vnet.ibm.com> writes:
> [...]
>>>> diff --git a/qemu-option.c b/qemu-option.c
>>>> index 27891e7..b83ca52 100644
>>>> --- a/qemu-option.c
>>>> +++ b/qemu-option.c
> [...]
>>>> static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>>>> @@ -832,14 +547,36 @@ void qemu_opts_del(QemuOpts *opts)
>>>>
>>>> int qemu_opts_print(QemuOpts *opts, void *dummy)
>>>> {
>>>> - QemuOpt *opt;
>>>> + QemuOpt *opt = NULL;
>>>> + QemuOptDesc *desc = opts->list->desc;
>>>>
>>>> - fprintf(stderr, "%s: %s:", opts->list->name,
>>>> - opts->id ? opts->id : "<noid>");
>>>> - QTAILQ_FOREACH(opt, &opts->head, next) {
>>>> - fprintf(stderr, " %s=\"%s\"", opt->name, opt->str);
>>>> + while (desc && desc->name) {
>>>> + opt = qemu_opt_find(opts, desc->name);
>>>> + switch (desc->type) {
>>>> + case QEMU_OPT_STRING:
>>>> + if (opt != NULL) {
>>>> + printf("%s='%s' ", opt->name, opt->str);
>>>> + }
>>>> + break;
>>>> + case QEMU_OPT_BOOL:
>>>> + printf("%s=%s ", desc->name, (opt && opt->str) ? "on" :
>>>> "off");
>>>> + break;
>>>> + case QEMU_OPT_NUMBER:
>>>> + case QEMU_OPT_SIZE:
>>>> + if (strcmp(desc->name, "cluster_size")) {
>>>> + printf("%s=%" PRId64 " ", desc->name,
>>>> + (opt && opt->value.uint) ? opt->value.uint : 0);
>>>> + } else {
>>>> + printf("%s=%" PRId64 " ", desc->name,
>>>> + (opt && opt->value.uint) ? opt->value.uint : desc->def_value);
>>>> + }
>>>> + break;
>>>> + default:
>>>> + printf("%s=(unknown type) ", desc->name);
>>>> + break;
>>>> + }
>>>> + desc++;
>>>> }
>>>> - fprintf(stderr, "\n");
>>>> return 0;
>>>> }
>>>>
>>>
>>> Why do you need to change this function?
>>
>> I just want to have the same output as before, and I noticed that
>> qemu_opts_print function
>> has no user. Is it Okay to change it?
>
> Can you give examples where unchanged qemu_opts_print() prints something
> else than your code?
>
After changing is:
[wdongxu]$ qemu-img create -f qcow2 t.qcow2 8G
Formatting 't.qcow2', fmt=qcow2 size=8589934592 encryption=off
cluster_size=65536 lazy_refcounts=off
If use qemu_otps_print directly:
[wdongxu]$ qemu-img create -f qcow2 t.qcow2 8G
qcow2-create-opts: <noid>: size="(null)"
Formatting 't.qcow2', fmt=qcow2
Without this patch, block.c uses print_option_parameters, output is as
the same as my code.
>>>> @@ -1110,3 +847,140 @@ int qemu_opts_foreach(QemuOptsList *list,
>> qemu_opts_loopfunc func, void *opaque,
>>>> loc_pop(&loc);
>>>> return rc;
>>>> }
>>>> +
>>>> +static size_t count_opts_list(QemuOptsList *list)
>>>> +{
>>>> + size_t i = 0;
>>>> +
>>>> + while (list && list->desc[i].name) {
>>>> + i++;
>>>> + }
>>>> +
>>>> + return i;
>>>> +}
>>>> +
>>>> +static bool opts_desc_find(QemuOptsList *list, const char *name)
>>>> +{
>>>> + const QemuOptDesc *desc = list->desc;
>>>> + int i;
>>>> +
>>>> + for (i = 0; desc[i].name != NULL; i++) {
>>>> + if (strcmp(desc[i].name, name) == 0) {
>>>> + return true;;
>>>
>>> Extra ;
>>>
>>>> + }
>>>> + }
>>>> + return false;
>>>> +}
>>>
>>> Duplicates the loop searching list->desc[] yet again. What about
>>> factoring out all the copies into a function? Separate cleanup patch
>>> preceding this one.
>>
>> Okay.
>>
>>>
>>>> +
>>>> +/* merge two QemuOptsLists to one and return it. */
>>>> +QemuOptsList *append_opts_list(QemuOptsList *first,
>>>> + QemuOptsList *second)
>>>> +{
>>>> + size_t num_first_options, num_second_options;
>>>> + QemuOptsList *dest = NULL;
>>>> + int i = 0;
>>>> + int index = 0;
>>>> +
>>>> + num_first_options = count_opts_list(first);
>>>> + num_second_options = count_opts_list(second);
>>>> + if (num_first_options + num_second_options == 0) {
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + dest = g_malloc0(sizeof(QemuOptsList)
>>>> + + (num_first_options + num_second_options) * sizeof(QemuOptDesc));
>>>
>>> Allocate space for both first and second.
>>
>> I just make sure we have enough to hold struct QemuOptsList and
>> QemuOptDesc members.
>
> Let me correct myself: allocate space for one new QemuOptsList plus
> enough space for the QemuOptDesc in first and second.
>
>>>> +
>>>> + if (first) {
>>>> + memcpy(dest, first, sizeof(QemuOptsList));
>>>> + } else if (second) {
>>>> + memcpy(dest, second, sizeof(QemuOptsList));
>>>> + }
>>>
>>> Copy either first or second.
>>>
>>> If both are non-null, the space for second remains blank.
>>>
>>> Why copy at all? The loops below copy again, don't they?
>>>
>>
>> struct QemuOptsList has a member "QemuOptDesc desc[]", what I want to do is:
>>
>> 1) Copy QemuOptsList, excluding "QemuOptDesc desc[]", because it is
>> "flexible array member",
>> does not take any space in QemuOptsList. Then dest will have the same
>> name and implied_opt_name
>> as first QemuoptsList's(if first is null, will be the same as second's).
>>
>> 2) Copy desc member, I have to allocate space for the "flexible array
>> member",
>> g_strdup name and help, if it already exists in desc, then skip g_strdup.
>>
>> Am I right?
>
> Your code confused me. Let me try again.
>
> dest->desc is set below. All the other members of dest get copied from
> first if it's non-null, else from second if it's non-null, else remain
> blank.
>
> Issues with copying from first or second:
>
> * Copying dest->head from is evil. It makes dest look like it was a
> member of tail queue dest->head.tqh_first, but that's not true.
>
> I figure you need to QTAILQ_EMPTY(&dest->head).
>
> * Copying dest->name isn't quite kosher, either, but probably harmless.
>
> * Copying dest->implied_opt_name isn't optimal. What if
> first->implied_opt_name is null, but second->implied_opt_name is
> non-null? Then dest->implied_opt_name = second->implied_opt_name
> would be nicer.
>
> I guess your particular use doesn't care, because all your
> implied_opt_name are null.
>
> * dest->merge_lists is fishy, too. It governs how multiple
> qemu_opts_create(dest, ...) with the same ID behave. What should
> happen if first->merge_lists != second->merge_lists?
>
> I guess all your merge_list are false.
>
> * When both first and second are null, *dest remains blank. Why is that
> okay?
>
> Perhaps a less problematic operation than "merge two QemuOptsLists"
> would be "set QemuOptsList C's desc to the merge of A's and B's desc."
> That way, the caller has to set up all the problematic members of the
> new QemuOptsList. Then use it to build a special function to create
> options for a pair of BlockDrivers.
>
Okay, you are right, I did not think about it so carefully. I will
follow your advice, using new name and initializing other struct members
in a new function.
>>>> +
>>>> + while (first && (first->desc[i].name)) {
>>>> + if (!opts_desc_find(dest, first->desc[i].name)) {
>>>> + dest->desc[index].name = strdup(first->desc[i].name);
>>>> + dest->desc[index].help = strdup(first->desc[i].help);
>>>
>>> g_strdup()
>>>
>>> Why do you need to copy name and help?
>
> No answer?
>
I want dest->desc has the same members as first plus second, so I copy
name and help, and desc->name will be used in opts_desc_find
function(see below).
>>>> + dest->desc[index].type = first->desc[i].type;
>>>> + dest->desc[index].def_value = first->desc[i].def_value;
>>>> + dest->desc[++index].name = NULL;
>>>
>>> I'd terminate dest->desc[] after the loop, not in every iteration.
>>
>> Okay.
>>
>>>
>>>> + }
>>>> + i++;
>>>> + }
>>>> + i = 0;
>>>> + while (second && (second->desc[i].name)) {
>>>> + if (!opts_desc_find(dest, second->desc[i].name)) {
>>>> + dest->desc[index].name = strdup(first->desc[i].name);
>>>> + dest->desc[index].help = strdup(first->desc[i].help);
>>>> + dest->desc[index].type = second->desc[i].type;
>>>> + dest->desc[index].def_value = second->desc[i].def_value;
>>>> + dest->desc[++i].name = NULL;
>>>> + }
>>>> + i++;
>>>> + }
>>>
>>> Please avoid duplicating the loop.
>>>
>>> What if first and second both contain the same parameter name?
>>
>> Then the second will be skipped, I used opts_desc_find to determine
>> whether it already exists or not.
>
> Ah, missed that, thanks.
>
> First argument's options take precedence over second's, same as in the
> function this replaces (append_option_parameters()). Okay.
>
> Document it in the function comment?
Okay, will add in v2.
>
>>>> + return dest;
>>>> +}
>>>> +
>>>> +void free_opts_list(QemuOptsList *list)
>>>> +{
>>>> + int i = 0;
>>>> +
>>>> + while (list && list->desc[i].name) {
>>>> + g_free((char *)list->desc[i].name);
>>>> + g_free((char *)list->desc[i].help);
>>>> + i++;
>>>> + }
>>>> +
>>>> + g_free(list);
>>>> +}
>>>> +
>>>> +void print_opts_list(QemuOptsList *list)
>>>> +{
>>>> + int i = 0;
>>>> + printf("Supported options:\n");
>>>> + while (list && list->desc[i].name) {
>>>> + printf("%-16s %s\n", list->desc[i].name,
>>>> + list->desc[i].help ? list->desc[i].help : "No description
>> available");
>>>> + i++;
>>>> + }
>>>> +}
>>>> +
>>>> +QemuOpts *parse_opts_list(const char *param,
>>>> + QemuOptsList *list, QemuOpts *dest)
>>>> +{
>>>> + char name[256];
>>>> + char value[256];
>>>> + char *param_delim, *value_delim;
>>>> + char next_delim;
>>>> +
>>>> + if (list == NULL) {
>>>> + return NULL;
>>>> + }
>>>> + while (*param) {
>>>> + param_delim = strchr(param, ',');
>>>> + value_delim = strchr(param, '=');
>>>> +
>>>> + if (value_delim && (value_delim < param_delim || !param_delim)) {
>>>> + next_delim = '=';
>>>> + } else {
>>>> + next_delim = ',';
>>>> + value_delim = NULL;
>>>> + }
>>>> +
>>>> + param = get_opt_name(name, sizeof(name), param, next_delim);
>>>> + if (value_delim) {
>>>> + param = get_opt_value(value, sizeof(value), param + 1);
>>>> + }
>>>> + if (*param != '\0') {
>>>> + param++;
>>>> + }
>>>> +
>>>> + if (qemu_opt_set(dest, name, value_delim ? value : NULL)) {
>>>> + goto fail;
>>>> + }
>>>> + }
>>>> +
>>>> + return dest;
>>>> +
>>>> +fail:
>>>> + return NULL;
>>>> +}
>>>
>>> Can you explain why the existing QemuOpts parsers won't do?
>>
>> I think exsiting parsers is for QEMUOptionParameter, I have not found
>> them for QemuOpts?
>
> Check out opts_do_parse() and its callers.
>
Okay.
>>>> diff --git a/qemu-option.h b/qemu-option.h
>>>> index ca72986..75718fe 100644
>>>> --- a/qemu-option.h
>>>> +++ b/qemu-option.h
>>>> @@ -31,24 +31,6 @@
>>>> #include "error.h"
>>>> #include "qdict.h"
>>>>
>>>> -enum QEMUOptionParType {
>>>> - OPT_FLAG,
>>>> - OPT_NUMBER,
>>>> - OPT_SIZE,
>>>> - OPT_STRING,
>>>> -};
>>>> -
>>>> -typedef struct QEMUOptionParameter {
>>>> - const char *name;
>>>> - enum QEMUOptionParType type;
>>>> - union {
>>>> - uint64_t n;
>>>> - char* s;
>>>> - } value;
>>>> - const char *help;
>>>> -} QEMUOptionParameter;
>>>> -
>>>> -
>>>> const char *get_opt_name(char *buf, int buf_size, const char *p,
>> char delim);
>>>> const char *get_opt_value(char *buf, int buf_size, const char *p);
>>>> int get_next_param_value(char *buf, int buf_size,
>>>> @@ -58,27 +40,6 @@ int get_param_value(char *buf, int buf_size,
>>>> int check_params(char *buf, int buf_size,
>>>> const char * const *params, const char *str);
>>>>
>>>> -
>>>> -/*
>>>> - * The following functions take a parameter list as input. This is
>> a pointer to
>>>> - * the first element of a QEMUOptionParameter array which is
>> terminated by an
>>>> - * entry with entry->name == NULL.
>>>> - */
>>>> -
>>>> -QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
>>>> - const char *name);
>>>> -int set_option_parameter(QEMUOptionParameter *list, const char *name,
>>>> - const char *value);
>>>> -int set_option_parameter_int(QEMUOptionParameter *list, const char *name,
>>>> - uint64_t value);
>>>> -QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
>>>> - QEMUOptionParameter *list);
>>>> -QEMUOptionParameter *parse_option_parameters(const char *param,
>>>> - QEMUOptionParameter *list, QEMUOptionParameter *dest);
>>>> -void free_option_parameters(QEMUOptionParameter *list);
>>>> -void print_option_parameters(QEMUOptionParameter *list);
>>>> -void print_option_help(QEMUOptionParameter *list);
>>>> -
>>>> /* ------------------------------------------------------------------ */
>>>>
>>>> typedef struct QemuOpt QemuOpt;
>>>> @@ -96,6 +57,7 @@ typedef struct QemuOptDesc {
>>>> const char *name;
>>>> enum QemuOptType type;
>>>> const char *help;
>>>> + uint64_t def_value;
>>>
>>> The only user I can see is qemu_opts_print(), which can't be right.
>>
>> I want to pass default value, such as QCOW2_DEFAULT_CLUSTER_SIZE, it
>> will be used
>> while qemu-img create, or qcow2_create can not get correct default
>> cluster size. Is it Okay?
>
> Maybe, but I can't see where def_value gets used, by qemu-img create or
> anything else. Please point me to the code that uses it.
>
Yep, it can be removed, I used such code to pass default value:
cluster_size = qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE,
DEFAULT_CLUSTER_SIZE);
>>>> } QemuOptDesc;
>>>>
>>>> struct QemuOptsList {
>>>> @@ -152,5 +114,10 @@ typedef int (*qemu_opts_loopfunc)(QemuOpts
>> *opts, void *opaque);
>>>> int qemu_opts_print(QemuOpts *opts, void *dummy);
>>>> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
>> void *opaque,
>>>> int abort_on_failure);
>>>> -
>>>> +QemuOptsList *append_opts_list(QemuOptsList *dest,
>>>> + QemuOptsList *list);
>>>> +void free_opts_list(QemuOptsList *list);
>>>> +void print_opts_list(QemuOptsList *list);
>>>> +QemuOpts *parse_opts_list(const char *param,
>>>> + QemuOptsList *list, QemuOpts *dest);
>>>> #endif
>>>
>>
>> Thank you Markus for you so detail comments, :)
>
> You're welcome.
>
>> Luiz, I think I need to use your 1-3 patchs in your series.
>> http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg02716.html
>
Thank you Markus.
