On 5/20/20 12:01 AM, Laszlo Ersek wrote: > On 05/19/20 20:20, Philippe Mathieu-Daudé wrote: >> The FW_CFG_DATA_GENERATOR allow any object to product > > (1) I suggest: > > s/allow/allows/ > s/product/produce/ > >> blob of data consumable by the fw_cfg device. >> >> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> >> --- >> include/hw/nvram/fw_cfg.h | 49 +++++++++++++++++++++++++++++++++++++++ >> hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++ >> 2 files changed, 79 insertions(+) >> >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h >> index 25d9307018..74b4790fae 100644 >> --- a/include/hw/nvram/fw_cfg.h >> +++ b/include/hw/nvram/fw_cfg.h >> @@ -9,11 +9,40 @@ >> #define TYPE_FW_CFG "fw_cfg" >> #define TYPE_FW_CFG_IO "fw_cfg_io" >> #define TYPE_FW_CFG_MEM "fw_cfg_mem" >> +#define TYPE_FW_CFG_DATA_GENERATOR_INTERFACE "fw_cfg-data-generator" >> >> #define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG) >> #define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) >> #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) >> >> +#define FW_CFG_DATA_GENERATOR_CLASS(class) \ >> + OBJECT_CLASS_CHECK(FWCfgDataGeneratorClass, (class), \ >> + TYPE_FW_CFG_DATA_GENERATOR_INTERFACE) >> +#define FW_CFG_DATA_GENERATOR_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(FWCfgDataGeneratorClass, (obj), \ >> + TYPE_FW_CFG_DATA_GENERATOR_INTERFACE) >> + >> +typedef struct FWCfgDataGeneratorClass { >> + /*< private >*/ >> + InterfaceClass parent_class; >> + /*< public >*/ >> + >> + /** >> + * get_data: >> + * @obj: the object implementing this interface >> + * >> + * Returns: pointer to start of the generated item data >> + */ >> + const void *(*get_data)(Object *obj); > > I'm not familiar with QOM, so please excuse any dumb questions. > > "const" suggests the blob returned remains owned by "obj";
Yes, it is owned by the generator object. fw_cfg_add_from_generator() does a memdup(). > that answers > the question whether the caller should attempt to free the blob. (The > answer is "no".) > > (2) However, will this perhaps expose other functions, currently taking > non-const-qualified pointers, to which we'd like to pass the blob > returned by the above member function? > > Because, then we'd have to cast away "const", It is illegal to cast away "const". This is why I choose to use it here, if the consumer want to modify it, it is forced to make its own copy. > and I find that much > uglier than removing the "const" from *here*, and adding a more verbose > comment as replacement. > > Yes, this is clearly speculation -- IOW just a question. If all the > functions we're going to pass the return value to are fine with > pointer-to-const, then this interface should be OK. The only user so far uses memdup(). > > (Obviously when I say "cast away const", I think of functions that do > not actually modify the object pointed-to by the non-const-qualified > pointer.) > >> + /** >> + * get_length: >> + * @obj: the object implementing this interface >> + * >> + * Returns: the size of the generated item data in bytes >> + */ >> + size_t (*get_length)(Object *obj); >> +} FWCfgDataGeneratorClass; >> + >> typedef struct fw_cfg_file FWCfgFile; >> >> #define FW_CFG_ORDER_OVERRIDE_VGA 70 >> @@ -263,6 +292,26 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char >> *filename, >> void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data, >> size_t len); >> >> +/** >> + * fw_cfg_add_from_generator: >> + * @s: fw_cfg device being modified >> + * @filename: name of new fw_cfg file item >> + * @generator_id: name of object implementing FW_CFG_DATA_GENERATOR >> interface >> + * @errp: pointer to a NULL initialized error object >> + * >> + * Add a new NAMED fw_cfg item with the content generated from the >> + * @generator_id object. The data referenced by the starting pointer is >> copied > > (3) s/referenced by the starting pointer/generated by the @generator_id > object/ > >> + * into the data structure of the fw_cfg device. >> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST >> + * will be used; also, a new entry will be added to the file directory >> + * structure residing at key value FW_CFG_FILE_DIR, containing the item >> name, >> + * data size, and assigned selector key value. >> + * >> + * Returns: the size of the generated item data on success, -1 otherwise. > > (4) I don't like ssize_t for a return value like this. > > First, get_length() returns size_t, which may not be representable in an > ssize_t. > > (Actually, it's worse than that; POSIX says, "the type ssize_t shall be > capable of storing values at least in the range [-1, {SSIZE_MAX}]" -- > and if I run "getconf SSIZE_MAX", I get 32767. Indeed, _POSIX_SSIZE_MAX, > which is the minimum for any implementation's SSIZE_MAX, is 32767.) > > Second, is a zero-sized blob useful in fw_cfg (from a generator)? Not for now. We can add it later anyway. > > If it is not useful, then this function should return size_t, and use > retval=0 for signaling an error. OK. > > If a zero-sized blob is useful, then the function should return a bool > (in addition to producing "errp"), and output the blob size as a > separate parameter. > >> + */ >> +ssize_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename, >> + const char *generator_id, Error **errp); >> + >> FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase, >> AddressSpace *dma_as); >> FWCfgState *fw_cfg_init_io(uint32_t iobase); >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >> index 8dd50c2c72..e18cb074df 100644 >> --- a/hw/nvram/fw_cfg.c >> +++ b/hw/nvram/fw_cfg.c >> @@ -1032,6 +1032,30 @@ void *fw_cfg_modify_file(FWCfgState *s, const char >> *filename, >> return NULL; >> } >> >> +ssize_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename, >> + const char *generator_id, Error **errp) >> +{ >> + FWCfgDataGeneratorClass *k; >> + Object *o; > > (5) Not sure about QEMU coding standards, but the above single-char > variable names (especially "o") terrify me. Please use "klass" and "obj". > > Do ignore my request if these variable names are just fine in QEMU. I don't want to terrify you :P > >> + size_t sz; >> + >> + o = object_resolve_path_component(object_get_objects_root(), >> generator_id); >> + if (!o) { >> + error_setg(errp, "Cannot find object ID %s", generator_id); >> + return -1; >> + } >> + if (!object_dynamic_cast(o, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) { >> + error_setg(errp, "Object '%s' is not a fw_cfg-data-generator >> subclass", >> + generator_id); > > (6) We should probably not open code > TYPE_FW_CFG_DATA_GENERATOR_INTERFACE as "fw_cfg-data-generator" even in > the error message. > > (7) If this branch is taken, would that arguably merit an assertion > failure? I mean, can the dynamic cast fail without QEMU having a related > bug somewhere? (Maybe this is going to be answered in the rest of the > series.) Because I see those OBJECT_CHECK macros near the top of > "fw_cfg.h", and those boil down to object_dynamic_cast_assert(). > >> + return -1; >> + } >> + k = FW_CFG_DATA_GENERATOR_GET_CLASS(o); >> + sz = k->get_length(o); >> + fw_cfg_add_file(s, filename, g_memdup(k->get_data(o), sz), sz); > > (g_memdup() takes a "guint" for "byte_size". Whether that matches > "size_t" is anyone's guess. I guess it can't be helped.) Similarly it returns a gpointer... > >> + >> + return sz; > > Right, this is the size_t --> ssize_t conversion that makes me > uncomfortable. > > I'm OK if you ignore all of my comments, these are simply the thoughts > that crossed my mind. Thanks for them! Regards, Phil. > > Thanks > Laszlo > >> +} >> + >> static void fw_cfg_machine_reset(void *opaque) >> { >> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); >> @@ -1333,12 +1357,18 @@ static const TypeInfo fw_cfg_mem_info = { >> .class_init = fw_cfg_mem_class_init, >> }; >> >> +static const TypeInfo fw_cfg_data_generator_interface_info = { >> + .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, >> + .parent = TYPE_INTERFACE, >> + .class_size = sizeof(FWCfgDataGeneratorClass), >> +}; >> >> static void fw_cfg_register_types(void) >> { >> type_register_static(&fw_cfg_info); >> type_register_static(&fw_cfg_io_info); >> type_register_static(&fw_cfg_mem_info); >> + type_register_static(&fw_cfg_data_generator_interface_info); >> } >> >> type_init(fw_cfg_register_types) >> >