[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v13 4/7] remus netbuffer: implement remus network buffering for nic devices
On 06/28/2014 01:54 AM, Ian Jackson wrote: Yang Hongyang writes ("[PATCH v13 4/7] remus netbuffer: implement remus network buffering for nic devices"):1.Add two members in libxl_domain_remus_info:Thanks. I'm going to make mostly general comments on this./* + * LIBXL_HAVE_REMUS_NETBUF 1 + * + * If this is defined, then the libxl_domain_remus_info structure will + * have a boolean field (netbuf) and a string field (netbufscript). + * + * netbuf, if true, indicates that network buffering should be enabled. + * + * netbufscript, if set, indicates the path to the hotplug script to + * setup or teardown network buffers. + */ +#define LIBXL_HAVE_REMUS_NETBUF 1This seems to imply that the default at the API level is for network buffering to be turned off. I don't think that's right, is it ?+/* If the device has a vifname, then use that instead of + * the vifX.Y format.I don't think it is right in general to look in the backend directory for the vifname. If we are using driver domains, the backend xenstore directory is writeable by the driver domain. But I think Remus is not at all compatible with driver domains right now - am I right ? If so then at the very least this function should have something in the comment saying that it must ONLY be used for remus because if driver domains were in use it would constitute a security vulnerability.+ ret = nl_cache_refill(netbuf_state->nlsock, netbuf_state->qdisc_cache); + if (ret < 0) { + LOG(ERROR, "cannot refill qdisc cache");Do these functions return error codes or set errno or something ? I think you should print the actual error that was encountered. That might be as simple as using LOGE instead but it depends on what they do. It returns a negative error code on error. + rc = libxl__xs_read_checked(gc, XBT_NULL, + GCSPRINTF("%s/hotplug-error", out_path_base), + &hotplug_error); + if (rc) { + rc = ERROR_FAIL;I think you can just "goto out" without setting rc again. That's true, will fix that. +static void netbuf_teardown_script_cb(libxl__egc *egc, + libxl__async_exec_state *aes, + int status)As I said in my other mail, can you reorganise this so that the functions are the file in the same order as the control flow, with appropriate comments dividing the different flows up ?+static void async_call_done(libxl__egc *egc, + libxl__ev_child *child, + pid_t pid, int status) +{ + libxl__remus_device *dev = CONTAINER_OF(child, *dev, child); + libxl__remus_device_state *rds = dev->rds; + STATE_AO_GC(rds->ao); + + if (WIFEXITED(status)) { + dev->callback(egc, dev, -WEXITSTATUS(status)); + } else { + dev->callback(egc, dev, ERROR_FAIL); + }This is really quite odd. These nonzero statuses are surely always errors ? Why leave it to the next layer to interpret ? Returning either a positive ERROR_* or a -WEXITSTATUS is particularly odd. And if you are going to do this you need to make a crystal-clear doc comment about it.+static void nic_match_async(const libxl__remus_device_ops *self, + libxl__remus_device *dev) +{ + if (dev->kind == LIBXL__REMUS_DEVICE_NIC) + _exit(0); + + _exit(-ERROR_NOT_MATCH); +} + +static void nic_match(libxl__remus_device_ops *self, + libxl__remus_device *dev) +{ + int pid = -1; + STATE_AO_GC(dev->rds->ao); + + /* Fork and call */ + pid = libxl__ev_child_fork(gc, &dev->child, async_call_done); + if (pid == -1) { + LOG(ERROR, "unable to fork"); + goto out; + } + + if (!pid) { + /* child */ + nic_match_async(self, dev); + /* notreached */ + abort();This is very odd. Why are you spawning a subprocess to test dev->kind ?+static void nic_setup(libxl__remus_device *dev) +{ + libxl__remus_device_nic *remus_nic; + libxl__remus_netbuf_state *ns = dev->ops->data; + const libxl_device_nic *nic = dev->backend_dev; + + STATE_AO_GC(ns->ao); + + GCNEW(remus_nic); + dev->data = remus_nic; + remus_nic->vif = get_vifname(dev, nic); + if (!remus_nic->vif) + goto out;I think you should set rc here rather than (implicitly) in out.+ setup_async_exec(&dev->aes, "setup", dev); + if (libxl__async_exec_start(gc, &dev->aes)) {And you should propagate the rc from libxl__async_exec_start. Yes, will fix that. +/* + * Note: This function will be called in the same gc context as + * libxl__remus_netbuf_setup, created during the libxl_domain_remus_start + * API call. + */ +static void nic_teardown(libxl__remus_device *dev)This comment is interesting but I don't understand the implications. Also isn't this a feature of the abstract/concrete API which should be documented there ?+static void nic_match_async(const libxl__remus_device_ops *self, + libxl__remus_device *dev) +{ + if (dev->kind == LIBXL__REMUS_DEVICE_NIC) + _exit(-ERROR_FAIL); + + _exit(-ERROR_NOT_MATCH);Perhaps each libxl__remus_device_ops could mention the kind it supports, so that the abstract code could check that ? That would avoid duplicating this check in every concrete kind's code.+extern libxl__remus_device_ops remus_device_nic;This needs to be const. I'm afraid I'm going to leave the drbd patch for another day... Thanks, Ian. . -- Thanks, Yang. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |