On 04/02/2013 08:22 AM, Michal Privoznik wrote:
> ---
> cfg.mk | 4 +-
> daemon/libvirtd-config.c | 12 +-
...
> src/util/virstatslinux.c | 1 -
> src/util/virstoragefile.c | 9 +-
Looks like most of the patch is fallout from new include file needed for
using existing functions in their new location,...
> src/util/virstring.c | 335 ++++++++++++++++++++++++++++
> src/util/virstring.h | 51 +++++
> src/util/virsysinfo.c | 8 +-
> src/util/virsysinfo.h | 2 +-
> src/util/virtime.c | 1 -
> src/util/virtypedparam.c | 3 +-
> src/util/viruri.c | 4 +-
> src/util/virusb.c | 7 +-
> src/util/virutil.c | 350 +-----------------------------
...and that the split of virutil.c into virstring.h is the meat of the
patch.
> 270 files changed, 1505 insertions(+), 1455 deletions(-)
Just from these numbers, I'm wondering if it would have been worth
splitting this patch into at least two, so that the code motion portion
of the patch is easier to find without having to hunt to the middle of
the giant patch. More thoughts on splitting below.
It is also possible to use 'git format-patch/send-email -O/path/to/file'
where /path/to/file contains a list of shell globs that control the
order in which the patch file is generated, so that the important
changes (src/util/virstring.[ch], src/util/virutil.[ch]) come first, and
the mechanical fallout last.
> +++ b/daemon/libvirtd-config.c
> @@ -23,15 +23,17 @@
>
> #include <config.h>
>
> +#include "configmake.h"
> #include "libvirtd-config.h"
> -#include "virconf.h"
> +#include "remote/remote_driver.h"
> +#include "remote/remote_protocol.h"
> +#include "rpc/virnetserver.h"
> #include "viralloc.h"
> +#include "virconf.h"
> #include "virerror.h"
> #include "virlog.h"
> -#include "rpc/virnetserver.h"
> -#include "configmake.h"
> -#include "remote/remote_protocol.h"
> -#include "remote/remote_driver.h"
> +#include "virstring.h"
> +#include "virutil.h"
You mixed sorting include names alongside adding the new include name,
which makes it a bit harder to follow, and makes the diff longer to
read. I would have split this into two to do include sorting first,
code motion and addition of include virstring.h second. But making you
redo things to split it now seems like a lot of work for little gain.
Oh well.
One thing is for certain - your commit message doesn't mention that you
did more than just add a new include. It took me this far into the
review to realize WHY your diff was so large (I was expecting lots of
1-line-additions for a new include, and was anticipating that things
like a +12/-11 would be due to use of a renamed function, only to find
out that you didn't rename any functions). Sorting includes is not
necessarily bad, but I should have found about it from the commit
message alone, not by diving into the patch itself.
> +++ b/src/conf/domain_nwfilter.h
> @@ -23,6 +23,8 @@
> #ifndef DOMAIN_NWFILTER_H
> # define DOMAIN_NWFILTER_H
>
> +# include "domain_conf.h"
> +
> typedef int (*virDomainConfInstantiateNWFilter)(virConnectPtr conn,
> const unsigned char *vmuuid,
> virDomainNetDefPtr net);
On the surface, this hunk feels completely unrelated. Obviously, it is
fallout from some other file changing includes to be in alphabetical
order, and it looks correct in isolation, but this is again evidence
that you crammed a bit much into one patch.
> +++ b/src/libvirt_private.syms
> @@ -1722,9 +1722,25 @@ virStorageFileResize;
>
>
> # util/virstring.h
> +virArgvToString;
> +virAsprintf;
> +virSkipSpaces;
> +virSkipSpacesAndBackslash;
> +virSkipSpacesBackwards;
> +virStrcpy;
> virStringFreeList;
> virStringJoin;
> virStringSplit;
> +virStrncpy;
> +virStrToDouble;
> +virStrToLong_i;
> +virStrToLong_l;
> +virStrToLong_ll;
> +virStrToLong_ui;
> +virStrToLong_ul;
> +virStrToLong_ull;
> +virTrimSpaces;
> +virVasprintf;
This list looks like a reasonable set of string-related functions.
While we generally like to name functions with a prefix that matches the
file it is found in, I agree that these names and this particular file
are common enough to be an exception to that rule (that is, requiring
someone to type something like virStrVasprintf would defeat the point of
having a convenient asprintf wrapper).
> +++ b/src/util/virstring.c
> @@ -21,6 +21,10 @@
>
> #include <config.h>
>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "c-ctype.h"
> #include "virstring.h"
> #include "viralloc.h"
> #include "virbuffer.h"
> @@ -166,3 +170,334 @@ void virStringFreeList(char **strings)
> }
> VIR_FREE(strings);
> }
> +
> +/* Like strtol, but produce an "int" result, and check more carefully.
> + Return 0 upon success; return -1 to indicate failure.
> + When END_PTR is NULL, the byte after the final valid digit must be NUL.
> + Otherwise, it's like strtol and lets the caller check any suffix for
> + validity. This function is careful to return -1 when the string S
> + represents a number that is not representable as an "int". */
> +int
> +virStrToLong_i(char const *s, char **end_ptr, int base, int *result)
My quick glance didn't spot any obvious flaws when doing the code motion.
> +char *
> +virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
> +{
> + char *ret;
> +
> + if (n > (destbytes - 1))
> + return NULL;
> +
> + ret = strncpy(dest, src, n);
> + /* strncpy NULL terminates iff the last character is \0. Therefore
> + * force the last byte to be \0
> + */
> + dest[n] = '\0';
Pre-existing - but since we already rejected too-large n, we are already
guaranteed that dest[n] is NUL without needing this statement. Don't
clean it up during the code motion; if you do clean it, it should be a
separate patch. Oh, and 'NUL' is the all-0-bit character (generally one
byte unless you are coding with wchar_t), NULL is the pointer (generally
multiple bytes), but there's enough comments in the code base that use
NULL where NUL was intended that I've given up trying to make them all
correct :)
> +++ b/src/util/virstring.h
> @@ -23,6 +23,7 @@
> # define __VIR_STRING_H__
>
> # include "internal.h"
> +# include <stdarg.h>
>
> char **virStringSplit(const char *string,
> const char *delim,
> @@ -35,4 +36,54 @@ char *virStringJoin(const char **strings,
>
> void virStringFreeList(char **strings);
>
> +int virStrToLong_i(char const *s,
> + char **end_ptr,
> + int base,
> + int *result);
Indentation is off, but looks like that was pre-existing in the old
location.
ACK with a better commit message. While I pointed out a few things that
are worth changing, those may be better as a followup patch (so that the
code motion is straight motion); and it would be quite time-consuming to
require you to split this into two patches.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list