Re: [libvirt] [PATCH v2 3/5] FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete().

On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote: Trying to figure the relationship between this socket and the file (eg, tapfd) created below). My comments from v1 still apply above. Since the "else" part of the above check is just replacing *ifname, then that's the only time we need to mess with *ifname. That is use "!=" and just replace *ifname at the end of the for loop as long as we generated one. The error message here would be wrong if you went through the existing "if" condition - it failed to strdup(). Thus you have: if (strstr(*ifname, "%d") != NULL) { int i; for (i = 0; i <= 255; i++) { virBuffer newname = VIR_BUFFER_INITIALIZER; virBufferAsprintf(&newname, *ifname, i); if (virBufferError(&newname)) { virBufferFreeAndReset(&newname); virReportOOMError(); goto cleanup; } if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { newifname = strdup(virBufferContentAndReset(&newname)); break; } } if (newifname) { VIR_FREE(*ifname); *ifname = newifname; } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to generate new name for interface %s"), ifr.ifr_name); goto cleanup; } } Also, I missed this the first time around, is there an existing constant for 255? Or is it/will it be dynamic? Beyond that I have some "thoughts" around the use of 255. First, it could be a time consuming loop - there's a lot of open, ioctl/check, close going on (and that doesn't include all the printf & Buffer mgmt code). Second, what are the chances some day someone wants 1024 tap devices. This loop is then outdated and even worse performance wise. It's too bad there isn't a way to just request the next available device and let the driver handle things rather than a linear algorithm which will cause performance problems some day. Yes, I've been down this road before. Assuming the above change, then 's/newifname/*ifname/' Removing the above too. The way the code is written now, if you goto cleanup from the SetName, then newifname is leaked. On success, we leave here with 's' and 'tapfd' being opened, right? Since the TapDelete will open another 's', should the "if (ret < 0)" above be removed and we always close 's'? If tapfd == NULL, then what happens? Shouldn't the device path still be opened & closed (like the "existing" code)? As it stands you return zero, but haven't necessarily opened the new dev_path. I'm trying to learn the relationship between the two. What happens when/if *tapfd == -1 because open() failed? and ret = 0?