Re: [Xen-devel] [PATCH] 1. changes for vdiskadm on illumos based platform

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sat, 2013-10-26 at 20:09 +0400, Igor Kozhkuhov wrote:
> 2. update ZFS in libfsimage from illumos for pygrub

This needs a Signed-off-by, please see
http://wiki.xenproject.org/wiki/Submitting_Xen_Patches for information. 

Also please construct the commit message as a single line summary (shown
in the short logs etc) followed by a longer description. The wiki has
some hints on this too.

also you seem to be mixing various different types of fixes (make fixes,
CFLAGS fixes, CC line fixes) which are then not well explained in the
commit log. It is best to split these out into separate changes. As it
stands this patch is huge and almost unreviewable.

> diff -r 7c12aaa128e3 -r c2e11847cac0 tools/libfsimage/Rules.mk
> --- a/tools/libfsimage/Rules.mk       Thu Oct 24 22:46:20 2013 +0100
> +++ b/tools/libfsimage/Rules.mk       Sat Oct 26 20:03:06 2013 +0400
> @@ -2,11 +2,19 @@ include $(XEN_ROOT)/tools/Rules.mk
>  
>  CFLAGS += -Wno-unknown-pragmas -I$(XEN_ROOT)/tools/libfsimage/common/ 
> -DFSIMAGE_FSDIR=\"$(FSDIR)\"
>  CFLAGS += -Werror -D_GNU_SOURCE
> +# need for build illumos ZFS
> +CFLAGS += -Wno-parentheses
> +CFLAGS += -Wno-unused

Should either be in zfs/Makefile or in config/SunOS.mk or at least added
based on CONFIG_SunOS then I think.

> +# end
>  LDFLAGS += -L../common/
>  
>  PIC_OBJS := $(patsubst %.c,%.opic,$(LIB_SRCS-y))
>  
> -FSDIR = $(LIBDIR)/fs
> +FSDIR-y = $(LIBDIR)/fs/$(FS)
> +FSDIR-$(CONFIG_SunOS)-x86_64 = $(PREFIX)/lib/fs/$(FS)/64
> +FSDIR-$(CONFIG_SunOS)-x86_32 = $(PREFIX)/lib/fs/$(FS)/
> +FSDIR-$(CONFIG_SunOS) = $(FSDIR-$(CONFIG_SunOS)-$(XEN_TARGET_ARCH))
> +FSDIR = $(FSDIR-y)

This seems to imply that LIBDIR is defined wrongly for the platform.
Shouldn't this be fixed in config/sunos.mk?
 
>  FSLIB = fsimage.so
>  
> @@ -15,11 +23,14 @@ fs-all: $(FSLIB)
>  
>  .PHONY: fs-install
>  fs-install: fs-all
> -     $(INSTALL_DIR) $(DESTDIR)$(FSDIR)/$(FS)
> -     $(INSTALL_PROG) $(FSLIB) $(DESTDIR)$(FSDIR)/$(FS)
> +     $(INSTALL_DIR) $(DESTDIR)$(FSDIR)
> +     $(INSTALL_PROG) $(FSLIB) $(DESTDIR)$(FSDIR)
> +
> +BUILD_LINE-y = $(CC) $(LDFLAGS) $(SHLIB_LDFLAGS) -o $@ $^ -lfsimage 
> $(FS_LIBDEPS) $(APPEND_LDFLAGS)
> +BUILD_LINE-$(CONFIG_SunOS) = $(CC) $(CFLAGS) $(LDFLAGS) $(SHLIB_LDFLAGS) -o 
> $@ $^ -lfsimage $(FS_LIBDEPS)

This seems to be some simple reordering plus (unintentionally?) dropping
$(APPEND_LFLAGS). Do you have reason to think that the reordering will
break on non-Illumos -- otherwise can't we just change it?

Oh, you add $(CFLAGS), why? This is a link invocation. Perhaps this
indicates that something is in CFLAGS which should be LDFLAGS?
 
>  $(FSLIB): $(PIC_OBJS)
> -     $(CC) $(LDFLAGS) $(SHLIB_LDFLAGS) -o $@ $^ -lfsimage $(FS_LIBDEPS) 
> $(APPEND_LDFLAGS)
> +     $(BUILD_LINE-y)
>  
>  clean distclean::
>       rm -f $(PIC_OBJS) $(FSLIB) $(DEPS)
> diff -r 7c12aaa128e3 -r c2e11847cac0 tools/libfsimage/common/Makefile
> --- a/tools/libfsimage/common/Makefile        Thu Oct 24 22:46:20 2013 +0100
> +++ b/tools/libfsimage/common/Makefile        Sat Oct 26 20:03:06 2013 +0400
> @@ -4,11 +4,16 @@ include $(XEN_ROOT)/tools/libfsimage/Rul
>  MAJOR = 1.0
>  MINOR = 0
>  
> -LDFLAGS-$(CONFIG_SunOS) = -Wl,-M -Wl,mapfile-SunOS
> +CFLAGS-ADDS-$(CONFIG_SunOS) += -Werror -Wp,-MD,.$(@F).d $(ADD_INCLUDES)
> +CFLAGS-ADDS-$(CONFIG_SunOS) += -I/usr/include/libxml2

This path should be detected (if it isn't already) by using pkgconfig
from the configure script. Or perhaps xml2-config is the right thing.
Either way hardcoding this here is wrong.

Some of the comments I made previous seem to apply to the rest of this
file too.
> [...]

> diff -r 7c12aaa128e3 -r c2e11847cac0 tools/libfsimage/common/fsimage.c
> --- a/tools/libfsimage/common/fsimage.c       Thu Oct 24 22:46:20 2013 +0100
> +++ b/tools/libfsimage/common/fsimage.c       Sat Oct 26 20:03:06 2013 +0400
> @@ -36,22 +36,43 @@
>  
>  static pthread_mutex_t fsi_lock = PTHREAD_MUTEX_INITIALIZER;
>  
> +#ifdef _VDISK_

What is _VDISK_? Where does it come from? I don't see it being defined
here, nor do I see vdisk.h being added.

There seems to be an awful lot of ifdeffery being added on the back of
this. Without knowing more about vdisk I can't advise fully but it seems
that some sort of refactoring would be preferable.

> +#include "vdisk.h"
> +#endif


> ++#ifdef _VDISK_
> +             if (ffi->ff_fsi->f_pvdisk) {
> +                     ret = vdisk_read(ffi->ff_fsi->f_pvdisk, (off_t)off,
> +                         buf, n);
> +             } else {
> +                     ret = pread(ffi->ff_fsi->f_fd, buf, n, off);
> +             }
> +#else
>               ret = pread(ffi->ff_fsi->f_fd, buf, n, off);
> +#endif

This pattern seems to have been repeated a lot. I think it should be
refactored into a function and expressed as

{
#ifdef _VDISK_
        if (...->f_pvdisk)
                return vdisk_read(...)
#endif
        return pread(...)
}

and equivalents for other ops like write etc. Or maybe a set of vdisk
ops which are implemented in terms of pread etc is the way to go? (I'd
need to know more about VIDSK to say)
> diff -r 7c12aaa128e3 -r c2e11847cac0 tools/libfsimage/common/fsimage_plugin.c
> --- a/tools/libfsimage/common/fsimage_plugin.c        Thu Oct 24 22:46:20 
> 2013 +0100
> +++ b/tools/libfsimage/common/fsimage_plugin.c        Sat Oct 26 20:03:06 
> 2013 +0400
> @@ -122,6 +122,7 @@ fail:
>  static int load_plugins(void)
>  {
>       const char *fsdir = getenv("FSIMAGE_FSDIR");
> +     const char *isadir = "";
>       struct dirent *dp = NULL;
>       struct dirent *dpp;
>       DIR *dir = NULL;
> @@ -130,8 +131,26 @@ static int load_plugins(void)
>       int err;
>       int ret = -1;
>  
> +#if defined(FSIMAGE_FSDIR)
>       if (fsdir == NULL)
>               fsdir = FSIMAGE_FSDIR;
> +#elif defined(__sun__)
> +     if (fsdir == NULL)
> +             fsdir = "/usr/lib/fs";
> +
> +     if (sizeof(void *) == 8)
> +             isadir = "64/";

Can't all this come from configure and/or config.mk for the platform?

Also, can't you just set fsdir to /usr/lib/fs or /usr/lib/fs/64 as
desired and avoid adding isadir?

> +#elif defined(__ia64__)

I think you must have carried this ia64 support over a rebase, it has
now been deleted.

> +     if (fsdir == NULL)
> +             fsdir = "/usr/lib/fs";
> +#else
> +     if (fsdir == NULL) {
> +             if (sizeof(void *) == 8)
> +                     fsdir = "/usr/lib64/fs";
> +             else
> +                     fsdir = "/usr/lib/fs";

This seems to have differing behaviour on non-SunOS from what was there
before. Another rebasing artefact perhaps?

> diff -r 7c12aaa128e3 -r c2e11847cac0 tools/libfsimage/zfs/fsi_zfs.h
> --- a/tools/libfsimage/zfs/fsi_zfs.h  Thu Oct 24 22:46:20 2013 +0100
> +++ b/tools/libfsimage/zfs/fsi_zfs.h  Sat Oct 26 20:03:06 2013 +0400
> @@ -36,6 +36,8 @@

There is a lot ZFS updates here. Are the resynchronising with some
existing upstream (perhaps grub?). If so please can you provide a
reference to the version sync'd in the commit log.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel