On Thu, Oct 27, 2022 at 01:00:45PM -0500, Jonathan Derrick wrote:
> +Parameters:
> +
> +``auto-ns-path=<path to nvme storage location>``
> + If specified indicates a support for dynamic management of nvme namespaces
> + by means of nvme create-ns command. This path points
> + to the storage area for backend images must exist. Additionally it requires
> + that parameter `ns-subsys` must be specified whereas parameter `drive`
> + must not. The legacy namespace backend is disabled, instead, a pair of
> + files 'nvme_<ctrl SN>_ns_<NS-ID>.cfg' and 'nvme_<ctrl SN>_ns_<NS-ID>.img'
> + will refer to respective namespaces. The create-ns, attach-ns
> + and detach-ns commands, issued at the guest side, will make changes to
> + those files accordingly.
> + For each namespace exists an image file in raw format and a config file
> + containing namespace parameters and state of the attachment allowing QEMU
> + to configure namespaces accordingly during start up. If for instance an
> + image file has a size of 0 bytes this will be interpreted as non existent
> + namespace. Issuing create-ns command will change the status in the config
> + files and and will re-size the image file accordingly so the image file
> + will be associated with the respective namespace. The main config file
> + nvme_<ctrl SN>_ctrl.cfg keeps the track of allocated capacity to the
> + namespaces within the nvme controller.
> + As it is the case of a typical hard drive, backend images together with
> + config files need to be created. For this reason the qemu-img tool has
> + been extended by adding createns command.
> +
> + qemu-img createns {-S <serial> -C <total NVMe capacity>}
> + [-N <NsId max>] {<path>}
> +
> + Parameters:
> + -S and -C and <path> are mandatory, `-S` must match `serial` parameter
> + and <path> must match `auto-ns-path` parameter of "-device nvme,..."
> + specification.
> + -N is optional, if specified it will set a limit to the number of potential
> + namespaces and will reduce the number of backend images and config files
> + accordingly. As a default, a set of images of 0 bytes size and default
> + config files for 256 namespaces will be created, a total of 513 files.
> +
> +Please note that ``nvme-ns`` device is not required to support of dynamic
> +namespaces management feature. It is not prohibited to assign a such device
> to
> +``nvme`` device specified to support dynamic namespace management if one has
> +an use case to do so, however, it will only coexist and be out of the scope
> of
> +Namespaces Management. NsIds will be consistently managed, creation
> (create-ns)
> +of a namespace will not allocate the NsId already being taken. If ``nvme-ns``
> +device conflicts with previously created one by create-ns (the same NsId),
> +it will break QEMU's start up.
By requiring the controller's serial number up-front, does this mean we
can't share dynamic namespaces with other controllers in the subsystem?
> +static inline char *create_fmt_name(const char *fmt, char *ns_directory,
> char *serial, uint32_t nsid, Error **errp)
> +{
> + char *file_name = NULL;
> + Error *local_err = NULL;
> +
> + storage_path_check(ns_directory, serial, errp);
> + if (local_err) {
How is 'local_err' ever *not* NULL here? Are you meaning to check
"*errp" instead?
> + error_propagate(errp, local_err);
> + } else {
> + file_name = g_strdup_printf(fmt, ns_directory, serial, nsid);
> + }
> +
> + return file_name;
> +}
> +
> +static inline char *create_cfg_name(char *ns_directory, char *serial,
> uint32_t nsid, Error **errp)
> +{
> + return create_fmt_name(NS_FILE_FMT NS_CFG_EXT, ns_directory, serial,
> nsid, errp);
> +}
> +
> +
> +static inline char *create_image_name(char *ns_directory, char *serial,
> uint32_t nsid, Error **errp)
> +{
> + return create_fmt_name(NS_FILE_FMT NS_IMG_EXT, ns_directory, serial,
> nsid, errp);
> +}
> +
> +static inline int nsid_cfg_save(char *ns_directory, char *serial, QDict
> *ns_cfg, uint32_t nsid)
> +{
> + GString *json = NULL;
> + char *filename;
> + FILE *fp;
> + int ret = 0;
> + Error *local_err = NULL;
> +
> + json = qobject_to_json_pretty(QOBJECT(ns_cfg), false);
> +
> + if (strlen(json->str) + 2 /* '\n'+'\0' */ > NS_CFG_MAXSIZE) {
> + error_setg(&local_err, "ns-cfg allowed max size %d exceeded",
> NS_CFG_MAXSIZE);
I find this whole "local_err" control flow unpleasant to follow. The
local_error gets set in this above condition only to be overwritten in
the very next step without ever being used? Surely that can't be right.
I'm just picking on this one example here, but the pattern appears to
repeat. I think this would be easier to read if the error conditions
took 'goto' paths to unwind so that the good path doesn't require such
deep 'if/else' nesting.
> + }
> +
> + filename = create_cfg_name(ns_directory, serial, nsid, &local_err);
> + if (!local_err) {
> + fp = fopen(filename, "w");
> + if (fp == NULL) {
> + error_setg(&local_err, "open %s: %s", filename,
> + strerror(errno));
> + } else {
> + chmod(filename, 0644);
> + if (!fprintf(fp, "%s\n", json->str)) {
> + error_setg(&local_err, "could not write ns-cfg %s: %s",
> filename,
> + strerror(errno));
> + }
> + fclose(fp);
> + }
> + }
> +
> + if (local_err) {
> + error_report_err(local_err);
> + ret = -1;
> + }
> +
> + g_string_free(json, true);
> + g_free(filename);
> + qobject_unref(ns_cfg);
> +
> + return ret;
> +}
